-
-
Notifications
You must be signed in to change notification settings - Fork 9.2k
Make time interval of datetime / time fields configurable #13597
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
Conversation
|
Thanks for opening the PR. 🚀 While I personally like the idea, I need to verify with the team first, whether we want this information as part of the schema. It will therefore take me a little to review the PR. |
packages/core/admin/admin/src/content-manager/components/Inputs/index.js
Outdated
Show resolved
Hide resolved
|
I think this would be better handled in the ctm "configure the view" and saved to the core store so it can be changed during runtime. Our general design policy prohibition storing visual changes like this in the schema. |
Codecov Report
@@ Coverage Diff @@
## main #13597 +/- ##
==========================================
- Coverage 55.50% 55.46% -0.04%
==========================================
Files 1274 1274
Lines 31823 31847 +24
Branches 5734 5745 +11
==========================================
+ Hits 17662 17664 +2
- Misses 12349 12364 +15
- Partials 1812 1819 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
We briefly discussed the PR today and would love to merge it, once all comments are resolved. Thank you for working on it - many users will appreciate it!
I've added a few comments and would to hear your opinion. 🚀
Could you please remove all unrelated changes from that PR? It looks like as if your prettier version/ config has some mismatch with what we are using (hence the brace / spacing changes).
Feel free to ping me anytime you need something - back from vacation now.
packages/core/admin/admin/src/content-manager/components/Inputs/index.js
Outdated
Show resolved
Hide resolved
...ages/core/content-type-builder/admin/src/components/FormModal/attributes/attributeOptions.js
Outdated
Show resolved
Hide resolved
...ages/core/content-type-builder/admin/src/components/FormModal/attributes/attributeOptions.js
Outdated
Show resolved
Hide resolved
...ages/core/content-type-builder/admin/src/components/FormModal/attributes/attributeOptions.js
Outdated
Show resolved
Hide resolved
packages/core/admin/admin/src/content-manager/components/Inputs/utils/getStep.js
Show resolved
Hide resolved
packages/core/content-type-builder/admin/src/translations/en.json
Outdated
Show resolved
Hide resolved
...ages/core/content-type-builder/admin/src/components/FormModal/attributes/attributeOptions.js
Outdated
Show resolved
Hide resolved
|
I did a refactor to get the step from the metadata instead of getting it from the schema @gu-stav |
|
@Jose4gg Thank you - I haven't checked it a lot yet, but it looks much cleaner. I'm wondering: does the field show up for you? |
|
@soupette Good catch - thanks. I keep confusing these two views. |
@gu-stav if he have added the steps in the metadata you should look into the setting page and not the ctb field configuration modal.
They should be merged into the CTB at some point... |
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've checked the PR again today: great work. I think we can almost merge it (there is a new release on Wednesday). I've added two smaller and one bigger comment and I'm interested what you think about it.
Thank you for still working on this! I know it has been a long time and I apologize for that.
packages/core/admin/admin/src/content-manager/pages/EditSettingsView/components/ModalForm.js
Show resolved
Hide resolved
packages/core/content-manager/server/controllers/validation/model-configuration.js
Outdated
Show resolved
Hide resolved
packages/core/admin/admin/src/content-manager/components/Inputs/index.js
Outdated
Show resolved
Hide resolved
|
Changes are done @gu-stav |
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 very much. It works like a charm and you did a really good job with the code. 🚀
I've already checked with the documentation team: this change only needs a small addition for now in the user-guide. Therefore I'll remove the documenation label and will create a new PR there.
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.
Rebased because the workflows were stuck.
step to datetime/time fields|
Thank you so much for your work 🥰🥰🥰 |
|
@Jose4gg thank you for this! |
|
@Coderwelsch @esiao Welcome, happy to contribute! 🙌 |
|
@esiao Thank you for sharing that insight. I agree it is not ideal - I will talk to the docs team next week to see how we can improve that situation. |
|
This pull request has been mentioned on Strapi Community Forum. There might be relevant details there: https://forum.strapi.io/t/decrease-the-time-granularity-on-datetime-picker/19769/3 |

What does it do?
With this PR It will allow to pass the step for time and datetime fields through [Model].json
Why is it needed?
There's some use cases where we don't need every minute to be an option in datetime/time fields
How to test it?
Adding
stepinside a datetime/time field of a [Model].json component should allow to override the number of minutes to be displayed in the content-manager.Related issue(s)/PR(s)
#12175
#5099