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(color-picker, time-picker): round corners, @HostListeners, openOnFocus for mccColorPickerOrigin, examples, time picker fixes #158

Merged
merged 12 commits into from
Aug 19, 2020

Conversation

motabass
Copy link
Contributor

implements #148

@motabass
Copy link
Contributor Author

motabass commented Aug 15, 2020

This also removes open Sans font and uses Roboto instead, aligns border-radius and elevation with what @angular/material uses for the datepicker (z4, and border-radius: 4px)

furthermore both color-picker and time-picker use the same button layout and default "Confirm" button text.

@motabass motabass force-pushed the master branch 2 times, most recently from 11d2f0e to 1843530 Compare August 15, 2020 21:07
@motabass motabass changed the title feat(color-picker): round corners, @HostListeners, openOnFocus for mccColorPickerOrigin, examples feat(color-picker, time-picker): round corners, @HostListeners, openOnFocus for mccColorPickerOrigin, examples Aug 15, 2020
@motabass motabass changed the title feat(color-picker, time-picker): round corners, @HostListeners, openOnFocus for mccColorPickerOrigin, examples feat(color-picker, time-picker): round corners, @HostListeners, openOnFocus for mccColorPickerOrigin, examples, time picker fixes Aug 15, 2020
@tiaguinho
Copy link
Owner

@motabass this is ready for review?

@tiaguinho tiaguinho linked an issue Aug 17, 2020 that may be closed by this pull request
@motabass
Copy link
Contributor Author

@tiaguinho yes

Copy link
Owner

@tiaguinho tiaguinho left a comment

Choose a reason for hiding this comment

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

@motabass In some methods, you have removed the documentation of params. There is some reason for that?

src/lib/timer-picker/timer-picker.component.scss Outdated Show resolved Hide resolved
src/lib/timer-picker/timer-picker.component.scss Outdated Show resolved Hide resolved
@motabass
Copy link
Contributor Author

motabass commented Aug 18, 2020

@tiaguinho i re-added the animation and the comment. I removed the @param comments because they seemed redundant to me when strongly typing it with TS.

The thing with the animation is, that one does not see it too much, because when hour is selected the view changes to minutes immediatly after the click. The same is true for the minutes when using the picker with hidden buttons. I was trying to add some delay, but i'm not sure if its a good idea really, also the test made problems then....what do you think?

Furthermore the animation is not going clockwise all the time and the selection-circle around the number is not animated which leads to looking strange while animating:

image

@tiaguinho tiaguinho added PR: in review target: master will be merge to master labels Aug 18, 2020
@motabass motabass requested a review from tiaguinho August 19, 2020 00:20
@tiaguinho
Copy link
Owner

tiaguinho commented Aug 19, 2020

Makes sense @motabass.
Let's remove the animation.

Please let me know when I can do the review again.

Thank you!

@motabass
Copy link
Contributor Author

motabass commented Aug 19, 2020

@tiaguinho I'm ready!

@tiaguinho tiaguinho merged commit 7d42519 into tiaguinho:master Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: in review target: master will be merge to master
Projects
None yet
2 participants