-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement week picker component #2768
Conversation
@martijnrusschen or someone else, can you take a look at this PR, and give your take on the questions I posed above? |
…ker into week-picker
Q2: I think the selecting of all dates in the week seems logical, but then I think you should not be able to individually select the dates. |
My question was actually more geared towards the internal state; each day component has a 'selected' and 'keyboard-selected' state, which I reuse for the week picker here. Another way would be to not consider these days selected or keyboard-selected in this case, but the week component that wraps these days. There's a bit of a trade-off here: the former option borrows heavily from the existing styling, meaning it doesn't introduce much complexity in the stylesheets, but is potentially less flexible. As to your comment: clicking on a particular date will currently select the corresponding date - do you mean to say that you don't agree with this? My issue with that is that when the week numbers are not present you won't have a meaningful thing to click on. A takeaway from this may be that the week numbers should always be shown when the week picker is used, but that is also currently not the case.
I agree, but wanted to lay the options on the table. 👍 Currently left/right behave the same as up/down in the week picker. |
I understood that the concept would be to select just weeks, so I thought there would be no need to select a single date, but here that single date would convert in to the week that the date belongs to. |
Correct, just having the week number select the first date of a week was already possible by using a callback on the week number. What this PR introduces is the additional logic required to properly display the state, changed keyboard navigation to match and a fallback that ensures the first day of a week is always used. |
Could some one take a look at why the specs are failing? I'm not really sure what's going wrong. |
Can you try to pull the latest master in? I think the issue was fixed
earlier.
…On Wed, 10 Mar 2021 at 16:34 TiesWestendorp ***@***.***> wrote:
Could some one take a look at why the specs are failing? I'm not really
sure what's going wrong.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2768 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKY2KGRGC53MJQRT7XBZOTTC6GQVANCNFSM4YIWJW4Q>
.
|
@martijnrusschen Could you still have a look at this PR? |
This looks good! Can you resolve the merge conflict so I can merge it in? I'm prepping a major release for this week and would love to get this in. |
👍 Done. |
I ran the tests, and it looks like there's some things breaking. Can you take a look? |
Should be fixed now. |
className={classnames(weekNumberClasses)} | ||
aria-label={`${ariaLabelPrefix} ${this.props.weekNumber}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this is removed completely?
@@ -0,0 +1,53 @@ | |||
import React from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking a bit on code coverage.. Might be good to add some tests that checks the behavior of showing/hiding the weekpicker as well. Making sure the weekpicker isn't shown when the prop isn't passed in and shown when it is passed in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov added some comments on coverage as well, would be great to improve the coverage to make sure we don't break this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's improve the test coverage to make sure we won't break this in the future.
Hello! Any news about week-picker? When can we expect a release? |
Any news on when we can get this merged? |
Why is it taking so long? |
Any update on this? |
When will this feature be merged? |
I really need this feature :) When will it be merged? |
Perhaps someone can take a look and bring this to the finish line. |
We are also currently interested in that feature. Is there something that outsider can do to help you out with making this happen or right now it's mostly in the reviewers' hands? |
Hey 👋 ! Any news about the merge? We really need this feature too :) Thank you in advance 🙏 |
Any update on this... really need this feature. Thanks in advance. |
Not sure if anyone needs this anymore but I was able to implement the week picker with the existing library... |
Hey @correahi! Thanks for this. I was wondering how you display the |
I don't have access to that codebase anymore but I'm pretty sure it's just a span with some text inside, something like |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Will this be merged into master? |
fixes #2033
Introduces the
showWeekPicker
setting, which allows picking a week. Internally, the date representing the week is the first day of that week. It's important that thedateFormat
prop matches thelocale
in the sense that they have the same starting day of the week. In the same vein, theselected
prop should refer to a date that's at the beginning of a week.Question 1: should left and right arrows navigate the months, rather than weeks? Answer: no.
Question 2: should the week component be considered selected/keyboard-selected, and the styling be derived from that, or should the days be considered selected/keyboard-selected like they currently are?