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

replaces ember-render-modifiers with custom modifier in DatePicker component #8312

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

stopfstedt
Copy link
Member

refs ilios/ilios#5374

components that use the DatePicker component:

stefan@nichtsnutz: ~/dev/projects/frontend on date-picker-modifier
$ egrep -r 'DatePicker' packages --include=*.hbs --exclude-dir=node_modules --exclude-dir=dist | egrep -v 'Rollover'
packages/ilios-common/addon/components/learningmaterial-manager.hbs:              <DatePicker
packages/ilios-common/addon/components/learningmaterial-manager.hbs:              <DatePicker
packages/ilios-common/addon/components/course/overview.hbs:              <DatePicker
packages/ilios-common/addon/components/course/overview.hbs:              <DatePicker
packages/ilios-common/addon/components/offering-form.hbs:            <DatePicker
packages/ilios-common/addon/components/session-overview-ilm-duedate.hbs:          <DatePicker
packages/frontend/app/components/curriculum-inventory/report-overview.hbs:              <DatePicker
packages/frontend/app/components/curriculum-inventory/report-overview.hbs:              <DatePicker
packages/frontend/app/components/curriculum-inventory/new-sequence-block.hbs:        <DatePicker
packages/frontend/app/components/curriculum-inventory/new-sequence-block.hbs:        <DatePicker
packages/frontend/app/components/curriculum-inventory/sequence-block-overview.hbs:              <DatePicker
packages/frontend/app/components/curriculum-inventory/sequence-block-overview.hbs:              <DatePicker
packages/frontend/app/components/my-profile.hbs:              <DatePicker

@stopfstedt stopfstedt force-pushed the date-picker-modifier branch 6 times, most recently from 6664ee8 to ea20ce3 Compare January 16, 2025 19:11
@stopfstedt stopfstedt force-pushed the date-picker-modifier branch from ea20ce3 to 5d2e82a Compare January 23, 2025 00:30
@stopfstedt stopfstedt marked this pull request as ready for review January 23, 2025 00:47
Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

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

Approved, with one take it or leave it comment.

packages/ilios-common/addon/modifiers/date-picker.js Outdated Show resolved Hide resolved
both files are 4k large uncompressed combined, gzip compressed it's a
little more than 1k. extracting these imports removes async/await
altogether from the task that sets up the picker. that's a worthwhile
tradeoff imo.
in the current version, changes to the locale are not registered by the
picker - we'll remain in English.
declaring the locale as input allows us to be responsive here.
the flatpickr locale handling is inconsistent - if you give it a locale
identifier, like we do by default with `en`, and then try to get that
back from the picker instance, you'll get an object.
handling it this way is much cleaner, at the expense of adding another property.
it also gets us around having to compare object to object for
non-english locales.
the isOpen tracked prop wasn't doing anything. it's wired up and
toggled, but its state never  gets checked.
BALETED.
it's been a while since this was kludged in. let's see if we can
consistently roll high without it now.
@stopfstedt stopfstedt force-pushed the date-picker-modifier branch from 5d2e82a to 8106566 Compare January 23, 2025 22:24
@stopfstedt stopfstedt requested a review from jrjohnson January 23, 2025 22:25
@stopfstedt stopfstedt force-pushed the date-picker-modifier branch from 8106566 to b59fb0e Compare January 23, 2025 22:25
@dartajax dartajax added the run ui tests Run the expensive UI tests label Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run ui tests Run the expensive UI tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants