fix(CalendarMonth) fix first day of month not showing when weekstart …#7679
Conversation
|
Preview: https://patternfly-react-pr-7679.surge.sh A11y report: https://patternfly-react-pr-7679-a11y.surge.sh |
thatblindgeye
left a comment
There was a problem hiding this comment.
Nice work on this! I did notice one issue that I noted below, just let me know what you think! 🙂
| let startOnWeek = 0; | ||
| if (firstDayOfWeek.getMonth() === defaultDate.getMonth() && firstDayOfWeek.getDate() !== 1) { | ||
| firstDayOfWeek.setDate(firstDayOfWeek.getDate() - 7); | ||
| startOnWeek = 1; | ||
| } | ||
| for (let i = startOnWeek; i < 6; i++) { | ||
| const week = []; |
There was a problem hiding this comment.
I'm noticing an issue when weekStart={3} is passed in and going to January 2030, where the beginning of the month gets rendered correctly, but the end of the month gets cut off:
It looks like it's caused by the for (let i = startOnWeek portion, as reverting that to i = 0 fixes it while also fixing the original issue at hand, at least in this specific scenario. Going to some other random dates seems to work fine as well.
There was a problem hiding this comment.
It seems that the end of some months is now getting cut off:
I think in order to really support this another row will need to be added to the calendar whenever this occurs, which we seem to do under some circumstances already:
Or alternatively, we could just always have the calendar show 6 weeks. I think that's what Google does in Google Calendar.
Also, I would like to see a test to prevent this behavior from regressing in the future.
|
@thatblindgeye and @mmenestr might have some insight into when we may or may not want 6 weeks displayed. I think they recently made some decisions regarding this topic together. |
|
Yes, we basically said that it should preferably vary based on whether the 6th week has any numbers from the current month. In other words, if the month needs 6 weeks to show up fully, then the calendar should show 6 weeks, but if the month only needs 5 weeks to show up fully (which would mean that the 6th week would have no dates from the month you're currently on, and instead only show calendar days for the following month) then only 5 weeks should show. I just thought it was weird that in a lot of cases, if we were to always show 6 weeks, you'd have a full line for dates that were not part of the month you were on. Hopefully that makes sense! |
8769cc8 to
4c2afe8
Compare
|
I fixed the last row not showing in specific cases, now it works as requested, so number of rows displayed varies based on whether the 6th week contains days from same month as selected. @wise-king-sullyman also shared some useful resources on testing with me, so I should be able to create tests to prevent this from regressing. |
4c2afe8 to
e616781
Compare
e616781 to
51a55a8
Compare
wise-king-sullyman
left a comment
There was a problem hiding this comment.
Two super small nits and then this will be good to merge!
| import { CalendarMonth } from '../CalendarMonth'; | ||
|
|
||
| test('Renders the first date in a month when a custom weekStart is passed', () => { | ||
| const formatAria = (date: Date) => |
There was a problem hiding this comment.
Can you add a comment here just stating that this custom aria label is needed because of a bug with automatic aria label generation as it is now, and that it can be removed once that bug is fixed?
There was a problem hiding this comment.
I think that'd it be actually a good thing to keep custom aria label because format of default generated aria label might change in future and the test would fail for seemingly unrelated reason.
There was a problem hiding this comment.
That's a fair point, though the same could be said for this approach and its reliance on the cellAriaLabel prop functioning properly. This line of thinking could also extend to many other behaviors of the component, such as the weekStart or date props working properly, or the structure of the component.
I totally get not wanting this test to be able to fail for something that isn't the actual behavior under test, but I think that it's generally reasonable for a test to rely on other behavior of the component under test since totally avoiding that would be next to impossible.
Plus if the generated aria labels are properly tested developers should be able to see that test failing as well if the generated aria label is changed, which should make it apparent what the true cause of the failure here is.
All of that being said I don't think what you've got here is bad by any means and I don't think it neccesarily needs to go, but I would still like a comment saying why it's here and that it shouldn't be needed once the bug is fixed 🙂
| expect(firstDate).toBeVisible(); | ||
| }); | ||
|
|
||
| test('Renders the first date in a month when a custom weekStart is passed', () => { |
There was a problem hiding this comment.
| test('Renders the first date in a month when a custom weekStart is passed', () => { | |
| test('Renders the last date in a month when a custom weekStart is passed', () => { |
51a55a8 to
f7e90be
Compare
wise-king-sullyman
left a comment
There was a problem hiding this comment.
🚀 This is really awesome work!
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
patternfly#7679) * fix(CalendarMonth) some weeks not showing when weekstart changed * fix(CalendarMonth) test added * fix(CalendarMonth) update tests
…set to monday
What: Closes #7654