feat(date-time-picker): add date/time input + picker #3856
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
13 tasks
Contributor
Tachometer resultsChromeaccordion permalinkbasic-test
action-bar permalinkbasic-test
action-button permalinkbasic-test
action-group permalinkbasic-test
action-menu permalinktest-basic
test-directive permalink
test-lazy permalink
test-open-close-directive permalink
test-open-close permalink
alert-banner permalinkbasic-test
alert-dialog permalinkbasic-test
asset permalinkbasic-test
avatar permalinktest-basic
badge permalinkbasic-test
banner permalinktest-basic
button-group permalinkbasic-test
button permalinktest-basic
Firefoxaccordion permalinkbasic-test
action-bar permalinkbasic-test
action-button permalinkbasic-test
action-group permalinkbasic-test
action-menu permalinktest-basic
test-directive permalink
test-lazy permalink
test-open-close-directive permalink
test-open-close permalink
alert-banner permalinkbasic-test
alert-dialog permalinkbasic-test
asset permalinkbasic-test
avatar permalinktest-basic
badge permalinkbasic-test
banner permalinktest-basic
button-group permalinkbasic-test
button permalinktest-basic
|
Westbrook
reviewed
Dec 14, 2023
Contributor
Westbrook
left a comment
Westbrook
left a comment
There was a problem hiding this comment.
Really great work so far here @mirekszot! Lots of notes and small changes, let me know if any need further explanation.
Comment on lines
+808
to
+847
| /** | ||
| * If the day being formatted is February 29th but the year segment has not yet been filled, we need to use a | ||
| * leap year to allow the 29th to remain, otherwise, if we use the current year and it is not a leap year, the | ||
| * day that would be displayed would be March 1st, as February 29th would not exist and JavaScript “moves” the | ||
| * day to the next day. As this year is only used to format the day and month, we use the year 2000 as the "base | ||
| * year" for formatting | ||
| */ |
Contributor
There was a problem hiding this comment.
🤯 How did you catch this!? Great work. Hopefully this is in the tests either in actuality or as a stub.
Contributor
There was a problem hiding this comment.
Is there a test or stub for this?
Author
Thanks @Westbrook for reviewing. All kudos should go to @paulodetarsofm, he's the author of this PR 👏 👏. |
mirekszot
force-pushed
the
feature/date-time-picker
branch
from
068d97f to
fa0b24f
Compare
mirekszot
force-pushed
the
mirekszot/date-time-picker-ui-fixes
branch
from
51dc62d to
e5d1a83
Compare
paulodetarsofm
force-pushed
the
feature/date-time-picker
branch
from
fa0b24f to
8df48a2
Compare
paulodetarsofm
force-pushed
the
mirekszot/date-time-picker-ui-fixes
branch
from
35279e2 to
e07f2a2
Compare
Westbrook
reviewed
Jan 12, 2024
Contributor
Westbrook
left a comment
Westbrook
left a comment
There was a problem hiding this comment.
Small questions and then we're good to merge this down to the project branch!
Comment on lines
+808
to
+847
| /** | ||
| * If the day being formatted is February 29th but the year segment has not yet been filled, we need to use a | ||
| * leap year to allow the 29th to remain, otherwise, if we use the current year and it is not a leap year, the | ||
| * day that would be displayed would be March 1st, as February 29th would not exist and JavaScript “moves” the | ||
| * day to the next day. As this year is only used to format the day and month, we use the year 2000 as the "base | ||
| * year" for formatting | ||
| */ |
Contributor
There was a problem hiding this comment.
Is there a test or stub for this?
Contributor
|
@paulodetarsofm Can you please let me know how can we help to get the below To-do's addressed? |
paulodetarsofm
force-pushed
the
feature/date-time-picker
branch
from
8df48a2 to
799aa94
Compare
* feat: add input segments * refactor: input segments improvements * fix(textfield): convert pseudo :read-only to attribute * feat(time-field): add new time field component * refactor: remove `_` from private properties and methods and convert property `locale` to a getter * refactor: add new `@internationalized/number` package * refactor: reduce cognitive complexity for `handleTypedValue()` method * "Refactor this function to reduce its Cognitive Complexity from 41 to the 15 allowed." (SonarLint) * refactor: reduce cognitive complexity for `handleClear()` method * "Refactor this function to reduce its Cognitive Complexity from 21 to the 15 allowed." (SonarLint) * refactor: reduce cognitive complexity for `setNewDateTime()` method * "Refactor this function to reduce its Cognitive Complexity from 30 to the 15 allowed." (SonarLint) * refactor: add correct placeholder for year * refactor: reduce cognitive complexity for `getSegmentValueAndLimits()` method * "Refactor this function to reduce its Cognitive Complexity from 34 to the 15 allowed." (SonarLint) * refactor: reduce cognitive complexity for `updateHour()` method * "Refactor this function to reduce its Cognitive Complexity from 21 to the 15 allowed." (SonarLint) * refactor: add missing comments and minor code improvements * refactor: preserve white spaces inside literal segments * refactor: add types to variables * refactor: formatting year * refactor: increment/decrement year when is `undefined` * feat: add logic to update day if needed * fix: improve logic to update the day * fix: package version * fix: add missing references * refactor: maintain alphabetical order * fix: duplicate identifier 'ClassInfo' * refactor: upgrade `@internationalized/date` package version used by the calendar * refactor: minor code improvements and comments * refactor: add new items to to-do list * refactor: add missing "time-field" entry * refactor: update packages version * refactor: use `import type` when possible * refactor: remove unnecessary configuration * refactor: remove unnecessary code * refactor: code and comments improvements * refactor: remove unused files * refactor: update `@spectrum-css/calendar` * refactor: move `input-segments` from `/packages` to `/tools` * fix: segment text colour * refactor(calendar): remove unnecessary code
* feat: add calendar * feat: use first day of the week according to the locale * feat: add "disabled" property * feat: handle "Previous" and "Next" month buttons * feat: add locale for all stories * feat: add "min" and "max" date * refactor: add new render methods * feat: format day using Intl.NumberFormat * fix: lit-plugin "no-incompatible-property-type" * The built in converter doesn't handle the property type Date | undefined * fix: packages version + property type error * refactor: add to-do list to README * feat: add event handlers and slots for icons * refactor(calendar): update dependencies * refactor(calendar): changing Lit resource import source * refactor: add comments and use correct formatters * refactor: convert property `_locale` to a getter * refactor: remove `_` from private properties and methods * refactor: remove unnecessary method * refactor: remove unnecessary template checking * refactor: add new private properties * refactor: code improvements, new comments * refactor: remove reflect from properties they don't need * fix: add new package to tsconfig.json * refactor: use same version of `@internationalized/number` used by other components * refactor(calendar): improvements and new TODOs
* feat: add input segments * refactor: input segments improvements * fix(textfield): convert pseudo :read-only to attribute * feat(time-field): add new time field component * refactor: remove `_` from private properties and methods and convert property `locale` to a getter * refactor: add new `@internationalized/number` package * refactor: reduce cognitive complexity for `handleTypedValue()` method * "Refactor this function to reduce its Cognitive Complexity from 41 to the 15 allowed." (SonarLint) * refactor: reduce cognitive complexity for `handleClear()` method * "Refactor this function to reduce its Cognitive Complexity from 21 to the 15 allowed." (SonarLint) * refactor: reduce cognitive complexity for `setNewDateTime()` method * "Refactor this function to reduce its Cognitive Complexity from 30 to the 15 allowed." (SonarLint) * refactor: add correct placeholder for year * refactor: reduce cognitive complexity for `getSegmentValueAndLimits()` method * "Refactor this function to reduce its Cognitive Complexity from 34 to the 15 allowed." (SonarLint) * refactor: reduce cognitive complexity for `updateHour()` method * "Refactor this function to reduce its Cognitive Complexity from 21 to the 15 allowed." (SonarLint) * refactor: add missing comments and minor code improvements * refactor: preserve white spaces inside literal segments * refactor: add types to variables * refactor: formatting year * refactor: increment/decrement year when is `undefined` * feat: add logic to update day if needed * fix: improve logic to update the day * fix: package version * fix: add missing references * refactor: maintain alphabetical order * fix: duplicate identifier 'ClassInfo' * refactor: upgrade `@internationalized/date` package version used by the calendar * refactor: minor code improvements and comments * refactor: add new items to to-do list * refactor: add missing "time-field" entry * refactor: update packages version * refactor: use `import type` when possible * refactor: remove unnecessary configuration * refactor: remove unnecessary code * refactor: code and comments improvements * refactor: remove unused files * refactor: update `@spectrum-css/calendar` * refactor: move `input-segments` from `/packages` to `/tools` * fix: segment text colour * refactor(calendar): remove unnecessary code
paulodetarsofm
force-pushed
the
mirekszot/date-time-picker-ui-fixes
branch
from
4bb3a27 to
2b74de8
Compare
Contributor
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
Rajdeepc
requested changes
Jul 31, 2024
mizgaionutalexandru
force-pushed
the
mirekszot/date-time-picker-ui-fixes
branch
from
8008530 to
c742b9a
Compare
Rajdeepc
reviewed
Aug 6, 2024
Contributor
Rajdeepc
left a comment
Rajdeepc
left a comment
There was a problem hiding this comment.
Good Progress! Few review comments then we can land this!
| "devDependencies": { | ||
| "@spectrum-css/calendar": "^4.2.4" | ||
| "@spectrum-css/calendar": "^4.2.4", | ||
| "@spectrum-web-components/story-decorator": "^0.41.2" |
Contributor
There was a problem hiding this comment.
I think this should be in dependencies and not in devDependencies. Do yo not need defaultLocale in prod?
| selectedDate?: Date; | ||
| min?: Date; | ||
| max?: Date; | ||
| padded?: boolean; |
Contributor
There was a problem hiding this comment.
this is not understandable, can you change it to something like calenderPadding instead
Contributor
There was a problem hiding this comment.
It is one of the component's attributes, I think we should stick to this name as the RFC decided.
| ?open=${this.open} | ||
| @sp-closed=${this.hidePicker} | ||
| > | ||
| <sp-popover class="popover"> |
Contributor
There was a problem hiding this comment.
FYI: This creates a stringent boundary when we would want to render this in mobile!
In mobile the calender should render in a tray, not sure if it has been design approved or not!
Contributor
There was a problem hiding this comment.
AFAIK there's no design for mobile. If the spectrum design team has something specific in mind we can do that. I agree that aligning with the React Spectrum approach (using the tray) should be desired but for the moment this approach can work. I can change this in a future PR before release and I think the MatchMediaController can be used here.
Contributor
There was a problem hiding this comment.
Right! This is just na FYI
| this.open = true; | ||
| } | ||
|
|
||
| public hidePicker(): void { |
Contributor
There was a problem hiding this comment.
We would love to have a documentation block!
|
|
||
| const dateTime = event.detail; | ||
|
|
||
| if (dateTime) { |
Contributor
There was a problem hiding this comment.
How are you handling the edge case here if dateTime is undefined?
| this.formatValue(this.monthSegment); | ||
|
|
||
| this.daySegment.value = day; | ||
| this.formatValue(this.daySegment); |
Contributor
There was a problem hiding this comment.
can you not do these formatValue early and do it at the last execution statements? Much cleaner approach
Contributor
|
Good to go into a feature branch! Please make the above suggested changes in your feature branch and we can take this forward. |
15 tasks
mizgaionutalexandru
added a commit
that referenced
this pull request
Dec 16, 2024
* feat: add input segments + time field (#3464) * feat: add input segments * refactor: input segments improvements * refactor: remove `_` from private properties and methods and convert property `locale` to a getter * refactor: add new `@internationalized/number` package * refactor: reduce cognitive complexity for `handleTypedValue()` method * "Refactor this function to reduce its Cognitive Complexity from 41 to the 15 allowed." (SonarLint) * refactor: reduce cognitive complexity for `handleClear()` method * "Refactor this function to reduce its Cognitive Complexity from 21 to the 15 allowed." (SonarLint) * refactor: reduce cognitive complexity for `setNewDateTime()` method * "Refactor this function to reduce its Cognitive Complexity from 30 to the 15 allowed." (SonarLint) * refactor: add correct placeholder for year * refactor: reduce cognitive complexity for `getSegmentValueAndLimits()` method * "Refactor this function to reduce its Cognitive Complexity from 34 to the 15 allowed." (SonarLint) * refactor: reduce cognitive complexity for `updateHour()` method * "Refactor this function to reduce its Cognitive Complexity from 21 to the 15 allowed." (SonarLint) * refactor: add missing comments and minor code improvements * refactor: preserve white spaces inside literal segments * refactor: add types to variables * refactor: formatting year * refactor: increment/decrement year when is `undefined` * feat: add logic to update day if needed * fix: improve logic to update the day * fix: package version * fix: add missing references * refactor: maintain alphabetical order * fix: duplicate identifier 'ClassInfo' * refactor: upgrade `@internationalized/date` package version used by the calendar * refactor: minor code improvements and comments * refactor: add new items to to-do list * refactor: add missing "time-field" entry * refactor: update packages version * refactor: use `import type` when possible * refactor: remove unnecessary configuration * refactor: remove unnecessary code * refactor: code and comments improvements * refactor: remove unused files * refactor: update `@spectrum-css/calendar` * refactor: move `input-segments` from `/packages` to `/tools` * fix: segment text colour * feat: add calendar * feat: use first day of the week according to the locale * feat: add "disabled" property * feat: handle "Previous" and "Next" month buttons * feat: add locale for all stories * feat: add "min" and "max" date * refactor: add new render methods * feat: format day using Intl.NumberFormat * fix: lit-plugin "no-incompatible-property-type" * The built in converter doesn't handle the property type Date | undefined * fix: packages version + property type error * refactor: add to-do list to README * feat: add event handlers and slots for icons * refactor: add comments and use correct formatters * refactor: convert property `_locale` to a getter * refactor: remove `_` from private properties and methods * refactor: remove unnecessary method * refactor: remove unnecessary template checking * refactor: add new private properties * refactor: code improvements, new comments * refactor: remove reflect from properties they don't need * fix: add new package to tsconfig.json * refactor: use same version of `@internationalized/number` used by other components * refactor: calendar improvements * feat: add input segments * refactor: input segments improvements * refactor: remove `_` from private properties and methods and convert property `locale` to a getter * refactor: add new `@internationalized/number` package * refactor: reduce cognitive complexity for `handleTypedValue()` method * "Refactor this function to reduce its Cognitive Complexity from 41 to the 15 allowed." (SonarLint) * refactor: reduce cognitive complexity for `handleClear()` method * "Refactor this function to reduce its Cognitive Complexity from 21 to the 15 allowed." (SonarLint) * refactor: reduce cognitive complexity for `setNewDateTime()` method * "Refactor this function to reduce its Cognitive Complexity from 30 to the 15 allowed." (SonarLint) * refactor: add correct placeholder for year * refactor: reduce cognitive complexity for `getSegmentValueAndLimits()` method * "Refactor this function to reduce its Cognitive Complexity from 34 to the 15 allowed." (SonarLint) * refactor: reduce cognitive complexity for `updateHour()` method * "Refactor this function to reduce its Cognitive Complexity from 21 to the 15 allowed." (SonarLint) * refactor: add missing comments and minor code improvements * refactor: preserve white spaces inside literal segments * refactor: add types to variables * refactor: formatting year * refactor: increment/decrement year when is `undefined` * feat: add logic to update day if needed * fix: improve logic to update the day * fix: package version * fix: add missing references * refactor: maintain alphabetical order * fix: duplicate identifier 'ClassInfo' * refactor: upgrade `@internationalized/date` package version used by the calendar * refactor: minor code improvements and comments * refactor: add new items to to-do list * refactor: add missing "time-field" entry * refactor: update packages version * refactor: use `import type` when possible * refactor: remove unnecessary configuration * refactor: remove unnecessary code * refactor: code and comments improvements * refactor: remove unused files * refactor: update `@spectrum-css/calendar` * refactor: move `input-segments` from `/packages` to `/tools` * fix: segment text colour * refactor: remove time field * refactor: move locales to be used globally * fix: picker button import * refactor: use `beforeinput` and `input` events * feat: add `date-time-picker` component * fix: use innerText to render segment text * fix: dealing with days and hours * fix: fix quiet variant styles * fix: add calculation to count the width of the date/time picker * refactor: removes solved TODO's points * refactor: removes broken unit test * refactor: adds unit test for default datetime picker * refactor: unit tests for date-time-picker * refactor: fixes input line height * chore: bump packages to 0.41.2 * refactor: feedback implementation v1 * refactor: mock slected date for timeGlanularity story * refactor: adds unit test to calendar * refactor: update naming * refactor: adds unit test for handleDayClick method * refactor: update TODO list * fix: create new segments when granularity is changed * fix: renaming attribute from `locale` to `lang` * refactor: use "quiet" variant of `sp-picker-button` when input is read-only * refactor: remove unused imports * refactor: enable "valid" and "invalid" attributes for all stories * refactor: italicised placeholders for non-CJK languages * refactor: date/time picker width definition * chore: bump packages to 0.41.2 * refactor: add comment about use of `.innerText` * refactor: reduce cognitive complexity * refactor: calendar improvements + unit tests * refactor: invalid picker button styles * chore: bump packages to 0.41.2 * refactor: small code improvements * chore: fix test imports * chore: update golden img hash * chore: update golden img hash --------- Co-authored-by: Paulo de Tarso Furtado Machado <paulodetarsofm@yahoo.com.br> Co-authored-by: Mirek Szot <ind34343@adobe.com> Co-authored-by: Mizga Ionut-Alexandru <mizgaionutalexandru@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Adding the third component that will be part of the date/time selector: the date/time field itself. The time field, which had been created in this PR, will no longer be used, inside the popover we will only have the calendar, at least for now.
Related issue(s)
Motivation and context
Allow the user to set the date and time to publish a post on social media.
How has this been tested?
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.