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

feat(stark-ui): implement stark-date-time-picker component #1270

Conversation

carlo-nomes
Copy link
Collaborator

@carlo-nomes carlo-nomes commented Apr 30, 2019

CLOSES ISSUE: #587

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #587

What is the new behavior?

Implemented DateTimePicker module and component that integrates with Reactive Forms and Angular Material MatFormField control.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@carlo-nomes carlo-nomes requested review from christophercr and SuperITMan and removed request for christophercr April 30, 2019 08:14
@carlo-nomes carlo-nomes force-pushed the feature/date-time-picker branch 2 times, most recently from b62cd8b to 7a40702 Compare April 30, 2019 08:46
@coveralls
Copy link

coveralls commented Apr 30, 2019

Coverage Status

Coverage increased (+0.03%) to 93.102% when pulling 6075e6a on cnomes:feature/date-time-picker into 9429f4f on NationalBankBelgium:master.

@christophercr christophercr removed the request for review from SuperITMan May 3, 2019 11:23
@christophercr christophercr force-pushed the feature/date-time-picker branch 6 times, most recently from a73fa8e to 190d281 Compare May 16, 2019 08:54
@christophercr christophercr added this to the 10.0.0-beta.8 milestone May 16, 2019
@christophercr christophercr requested a review from SuperITMan May 16, 2019 09:24
@christophercr
Copy link
Collaborator

@SuperITMan @cnomes I've updated this PR with the "finished" implementation of the DateTime picker.

Can you have a look? There is still one small fix to do in the styling and also the unit tests need to be expanded, but in terms of functionality the feature is done 😉 (or I hope so).

@christophercr christophercr changed the title feat(stark-ui): add stark-date-time-picker component feat(stark-ui): implement stark-date-time-picker component May 16, 2019
Copy link
Collaborator Author

@carlo-nomes carlo-nomes left a comment

Choose a reason for hiding this comment

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

A couple of remarks:

  • when you disable an empty picker the icons get shown again.
  • on the example page if you have more than 1 error they overlap with the button below
  • and the clear button moves everything around.

Also I think a nice feature would be that when you press tab when in the date input you focus the time input

@carlo-nomes
Copy link
Collaborator Author

I'm really liking how its working. 👍

@christophercr christophercr force-pushed the feature/date-time-picker branch 3 times, most recently from 091493b to d3ed804 Compare May 29, 2019 15:17
@christophercr
Copy link
Collaborator

christophercr commented May 29, 2019

@SuperITMan @cnomes I've updated the PR with all the styling fixes and unit tests. I also applied the changes based on your last remarks. However I have some concerns about the following remarks:

when you disable an empty picker the icons get shown again.

In fact this is the same behavior in the Material Date Picker, so I think we should keep it like that in order to be consistent.

Also I think a nice feature would be that when you press tab when in the date input you focus the time input

I don't think this is a good idea in terms of UX because the user should be able to display the calendar also with the keyboard. So if we change the tabbing order, it will be confusing for the user.

Can you guys review again this PR?

@christophercr
Copy link
Collaborator

@cnomes @SuperITMan can you guys have a look on this PR? I've done all the fixes mentioned in your remarks. This way we can release today or tomorrow the latest 😉

@carlo-nomes
Copy link
Collaborator Author

Still looks good to me. 👍Is the plan still to release today?

@christophercr
Copy link
Collaborator

Mmm, most likely tomorrow :p

@christophercr christophercr force-pushed the feature/date-time-picker branch from d3ed804 to 6075e6a Compare June 5, 2019 13:21
@christophercr
Copy link
Collaborator

@SuperITMan I've updated this PR with the small fix that you mentioned: the time field can now be entirely cleared manually and type a different time ;)

Can you have a look?
We are almost there!! Hopefully this can be merged now so we can release today! 😉

@christophercr christophercr merged commit a283991 into NationalBankBelgium:master Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants