-
Notifications
You must be signed in to change notification settings - Fork 56
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
V2 addon format and switch to pnpm, change localization and styles setup #1911
Conversation
@AmauryD yeah, we will need to tell people to import the themes and languages directly, which I think is fine. |
Maybe flatpickr should be be a peerDep as well, so people ship all their own flatpickr stuff, but we provide the components on top. |
I agree, let's do this ! |
@RobbieTheWagner I'm having a hard time figuring how to tell the user to import themes/default locales correctly. Does this seems ok to you ? https://github.com/shipshapecode/ember-flatpickr/blob/8ea4e52ba814e3ace92eff85077f5c0d626f9c16/test-app/app/templates/docs/usage.md |
I'm currently testing this PR in one of my company's project. Seems to be working fine. Just some documentation to finish and it's ready. |
@AmauryD what's the status on this? |
@RobbieTheWagner should be ready for review ! |
automerge: | ||
needs: [test, floating, try-scenarios] |
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.
@AmauryD can we add back the ember-try scenarios please?
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.
is it necessary ? It's already tested by the test command. https://github.com/AmauryD/ember-flatpickr/blob/main/test-app/config/ember-try.js
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.
The ember-try setup should remain as close to how it was before as possible for ease of upgrading.
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, I don't know if it runs in CI at all now. We want each try scenario to be a separate run so we can see which ones pass and fail.
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 understand. I'll fix this after work 😄
Thank's for the feedback by the way
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.
Thank you for all your hard work on 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.
The scenarios are back !
|
||
# misc | ||
/.env* |
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 think pretty much everything should stay as it was in this file and the old contents should be copied to the new ember-flatpickr folder, I think, to ensure the right things are ignored. Especially stuff like ember-try, which we want to keep.
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.
Just to be sure, you prefer to keep the original content of this file at the project root ?
Downgraded test helpers from 3.x to 2.x to pass compatibility of ember-lts-4.4 |
Merry PR Chistmas 🎅 ⛄ |
yarn add -D flatpickr | ||
``` | ||
|
||
### Default style |
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.
Should we mention themes or is that mentioned elsewhere?
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.
There's a brief section on this page: https://github.com/RobbieTheWagner/ember-flatpickr/blob/2b148eb050bbce62026f38dcf108ca085bcf49a5/test-app/app/templates/docs/usage.md#themes
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.
Awesome work, thank you! There might be a few things we need to document a bit more, but I want to merge this so we don't keep getting conflicts.
You're welcome ! It was a pleasure ! 😄 |
Related issue: #1910
@RobbieTheWagner I began to work on this, but this will be a breaking change:
The theme and language part of flatpickr will be impacted.