Skip to content
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

DateInput2 #1556

Merged
merged 1 commit into from
Jul 16, 2024
Merged

DateInput2 #1556

merged 1 commit into from
Jul 16, 2024

Conversation

balzss
Copy link
Contributor

@balzss balzss commented Jun 20, 2024

test plan:

  • check the example for DateInput2 with and without the year picker -> try if works as expected
  • try out some props
  • read docs if it makes sense
  • check a11y
  • check the notice on the old DateInput docs

Closes: INSTUI-4097

@balzss balzss requested review from matyasf and HerrTopi June 20, 2024 17:47
@balzss balzss self-assigned this Jun 20, 2024
Copy link

Preview URL: https://1556--preview-instui.netlify.app

@balzss balzss changed the title feat(ui-date-input): add year picker to DateInput and note to use the new api DateInput2 Jul 3, 2024
@balzss balzss requested review from matyasf and HerrTopi July 8, 2024 13:52
@balzss balzss force-pushed the feat/dateinput-yearpicker branch from 04ec792 to 4b5ad11 Compare July 8, 2024 14:27
Copy link
Collaborator

@matyasf matyasf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • when I enter an invalid date in the first example the invalid date message flashes for a second but then disappears
  • as I mentioned before it should be nice if I could move with arrows in calendar dates, but as I understand this needs to happen in the Calendar component.

Mz biggest concern that its using moment which is an out of date library. if we switch to a more modern lib, it will be a breaking change for our users.
We should investigate how a polyfill for the new Temporal API can replace moment for us.

@balzss balzss force-pushed the feat/dateinput-yearpicker branch from 4b5ad11 to 6927924 Compare July 10, 2024 12:00
@balzss
Copy link
Contributor Author

balzss commented Jul 10, 2024

  • when I enter an invalid date in the first example the invalid date message flashes for a second but then disappears
  • as I mentioned before it should be nice if I could move with arrows in calendar dates, but as I understand this needs to happen in the Calendar component.

Mz biggest concern that its using moment which is an out of date library. if we switch to a more modern lib, it will be a breaking change for our users. We should investigate how a polyfill for the new Temporal API can replace moment for us.

  1. good catch, this is fixed now
  2. yes, this should be done in Calendar
  3. regarding moment: end users (devs) don't really interact or depend on moment because this component simply returns a date string (just like the original DateInput) but is uses moment internally to parse and validate the dates. both of these can be replaced somewhat easily with the native Date object but that causes a small mismatch in the validation of this and Calendar component. this is addressed in the code with a comment and it won't change anything for users when we remove it later

@balzss balzss requested a review from matyasf July 10, 2024 12:13
@balzss balzss force-pushed the feat/dateinput-yearpicker branch 3 times, most recently from fbe250e to 966a08c Compare July 12, 2024 15:57
@balzss balzss requested a review from joyenjoyer July 15, 2024 08:45

const handleDateSelected = (
dateString: string,
_momentDate: any, // real type is Moment but used any to avoid imporing the moment lib
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: imporing

@joyenjoyer
Copy link
Contributor

When I set size to either small or medium, no height or width changes only when it is set to large. Is that an expected behavior?

@balzss
Copy link
Contributor Author

balzss commented Jul 15, 2024

When I set size to either small or medium, no height or width changes only when it is set to large. Is that an expected behavior?

nice catch! the icon wasn't changing size with the input so it prevented it to become small, now it's fixed

Copy link
Contributor

@joyenjoyer joyenjoyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice job!

Copy link
Collaborator

@matyasf matyasf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, very nice work!
FYI we have a @isWIP tag that you can use in our markdown files that wont make them appear in the sidebar (but you can see it by typing the URL)

@balzss
Copy link
Contributor Author

balzss commented Jul 16, 2024

looks good, very nice work! FYI we have a @isWIP tag that you can use in our markdown files that wont make them appear in the sidebar (but you can see it by typing the URL)

good to know but I'm not sure if we want that since the component can more or less replace most of the current uses of the original DateInput and we explicitly recommend it in the docs

@balzss balzss force-pushed the feat/dateinput-yearpicker branch from 544a761 to 608dc2d Compare July 16, 2024 11:48
@balzss balzss merged commit 9c893fc into master Jul 16, 2024
8 checks passed
@balzss balzss deleted the feat/dateinput-yearpicker branch July 16, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants