Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feat: New Date Picker Design #15343
Feat: New Date Picker Design #15343
Changes from all commits
23c1c2f
07f7803
55da3eb
d78625c
eaffefb
974dfa2
1406300
95d58f4
57dad19
bba8154
84354f1
687828c
883070e
26ce073
d99dbd9
87c49be
cbac7b4
fee5aad
90e3732
b319be0
6563d39
1d5e894
1f70100
49b8343
d0312f0
cc89e58
0eed6c5
cffa5eb
d47f559
b7c0b8b
d98d5bc
2823d78
71a8894
97300ed
e17531c
6299383
9cd301c
71824b9
f087628
91fc8d2
59e0f72
beaa24c
cc4300f
dd735b1
ce75a1b
498876f
989494c
facdfcd
e1fc14a
5ce634a
dfd755f
fed133a
88bd92a
0c4b043
2a81ce1
d8cbec4
4e1ffd1
e184109
e14c37b
8b58c54
551bf6c
92a8687
0f3164a
dfc8714
2968bd1
46fd131
d125aee
673cc7c
ff94600
bff8fb1
c0fd2a1
5e78efe
edf6cca
853def8
2abbce4
f676d87
9307c38
2a60148
3270817
be94db1
2645f7e
f3e4d19
7ea4962
91f5ae8
bb755bf
91684e1
48f0b14
406de1b
ff4043a
eaf012b
9016573
880d91b
9afdd53
76e1d0e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Also super NAB - i feel like
backTo
could be slightly clearer, so maybe we can explain in a comment or change tooriginatingRoute
or something like that? If y'all agree it's not immediately clearThere 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.
There's a huge amount of comment history in this PR, so apologies if I missed something, but imo this
CalendarPicker
component should not have been separated from theNewDatePicker
. Doing so actually makes things a bit more complicated.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.
I agree. It would have been good to get some wider input before introducing large changes like this.
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.
It was separated to two components as initially we had a logic which was hiding and opening the date picker once user focused into the date input. So it was clearer from developer perspective that way I would say.
Down the road we have decided to remove this logic and the calendar is permanently shown now so it should be easier to keep it all in one component now
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.
Made an issue to track the improvements here if you want to follow along
There was a bunch of follow up PRs to this component since this PR got merged so I am sure we could use some clean up and refactoring as well.
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.
This caused a regression.
Text wasn't changing when switched to a different locale through another device/tab
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.
This comparison was not accurate. More details about the root cause: #28622 (comment)