-
Notifications
You must be signed in to change notification settings - Fork 89
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
Updated Datepicker component #127
Conversation
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.
Hello there yashsehgal 👋
Thank you and congrats 🎉 for opening your first PR on this project.✨
We will review it soon! Till then you can checkout the README.md
for more details on it.
Moja global fosters an open and welcoming environment for all our contributors.🌸 Please adhere to our Code Of Conduct.
Feel free to join us on moja global Private Slack by dropping an email here.👩💻
Thanks @yashsehgal! Just a comment - most of the time users will be running simulations for many years (decades, even centuries). It's probably more important to dial in the year and month, rather than a specific day. If I'm being lazy, I normally just leave my simulations to the first day of each month. There are some occasions where getting the day right is also important so it's tough to balance! Not sure if that changes your design thinking. The component otherwise looks great! |
Thanks for adding this point @aornugent You're correct about this . Maybe we can create component where by-default users can added month and year, and if they want more precision then they can add a date as well by clicking an If that approach seems good, I can start creating a short UI design for this component. |
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.
Hey @yashsehgal , this implementation looks great. I have provided a small suggest wrt prop definition of the component, after that I guess it is good to be merged.
@@ -17,6 +25,9 @@ | |||
import moment from 'moment' | |||
|
|||
export default { | |||
props: { | |||
datepickerWidth: String |
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 can just rename datePickerWidth
as size
.
Also, it would be better if we can have:
- Default Prop for size (medium)
- Validator function to check if the value is from ["small", "medium", "large"] (refer Vue3 docs for Props)
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.
Sure @sohamsshah Thanks for the suggestions, I will fix these issues and add the listed features 👍🏽
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.
Hey @sohamsshah I have done the listed changes, you can review this PR now 👍🏽
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 the Validator function also added?
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.
LGTM! 🎉🙌
Thanks for working on this @yashsehgal !
Pull Request Template
Description
Date Picker
Component #112Testing
I have used this date-picker component on code-sandbox as well before implementing it. You can see that output here
Additional Context (Please include any Screenshots/gifs if relevant)
...