Skip to content

Feat/improve time picker behavior#1827

Merged
rustem-nasyrov merged 43 commits intoepicmaxco:developfrom
rustem-nasyrov:feat/improve-time-picker-behavior
Jun 30, 2022
Merged

Feat/improve time picker behavior#1827
rustem-nasyrov merged 43 commits intoepicmaxco:developfrom
rustem-nasyrov:feat/improve-time-picker-behavior

Conversation

@rustem-nasyrov
Copy link
Copy Markdown
Contributor

@rustem-nasyrov rustem-nasyrov commented May 20, 2022 •
edited by RVitaly1978
Loading

Changes

  • Selection is always centered.
    image
  • Scrolling should automatically select time.
time-picker-scroll.mp4
  • Refactoring.
  • Added new property to set the height of the time cell.
  • Added new property to visualize selection: framed.
    image
  • Added visible cells count property.
    image

Docs

  • Added translation to russian.
  • Wrote description for the new properties and translation for it.

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Improvement/refactoring (non-breaking change that doesn't add any feature but make things better)

Close #1549.

m0ksem
m0ksem requested changes May 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@m0ksem m0ksem left a comment

Choose a reason for hiding this comment

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

Looks good. I worry about scroll animation and lots of constants.

@m0ksem m0ksem added the feature Something useful to end user label May 20, 2022
@m0ksem m0ksem added this to the 1.4.1 milestone May 20, 2022
@rustem-nasyrov rustem-nasyrov requested a review from m0ksem May 23, 2022 11:49
m0ksem
m0ksem requested changes May 23, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@m0ksem m0ksem left a comment

Choose a reason for hiding this comment

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

Looks like everything is broken now:

image
firefox_puziZ8O5Mf

asvae
asvae requested changes May 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@asvae asvae left a comment

Choose a reason for hiding this comment

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

Code-wise looks nice, but needs some work with implementation. :)

@rustem-nasyrov rustem-nasyrov requested a review from asvae May 27, 2022 12:50
Derranion
Derranion reviewed May 27, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Derranion Derranion left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Scrolling seems a bit buggy when cursor is hovered on hours \ minutes.

Rolls back often.

@Derranion
Copy link
Copy Markdown
Member

Derranion commented May 27, 2022

Am \ pm toggle makes picker select incorrect values.

Derranion
Derranion requested changes May 30, 2022
View reviewed changes
packages/docs/src/locales/en/en.json
@@ -1134,12 +1134,15 @@
},
"VaTimePicker": {
Copy link
Copy Markdown
Member

@Derranion Derranion May 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Here are some examples of this:

Clicked on 8 hours, 9 hours are selected as the result.

incorrecetTimeSelection

Copy link
Copy Markdown
Member

@Derranion Derranion Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Update:
Now bug shows only when clicking a few times at the bottom.

Copy link
Copy Markdown
Member

@Derranion Derranion Jun 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Still relevant - you need to scroll down with am\pm mode active and select 8 hours.

packages/docs/src/locales/en/en.json
@@ -1134,12 +1134,15 @@
},
"VaTimePicker": {
Copy link
Copy Markdown
Member

@Derranion Derranion May 30, 2022

Choose a reason for hiding this comment

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

While scrolling TimePicker it may roll back from time to time unpredictably.

scrolledDownRolledBackSelectedTime

Copy link
Copy Markdown
Member

@Derranion Derranion Jun 13, 2022

Choose a reason for hiding this comment

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

Still relevant

packages/docs/src/locales/en/en.json
@@ -1134,12 +1134,15 @@
},
"VaTimePicker": {
Copy link
Copy Markdown
Member

@Derranion Derranion May 30, 2022

Choose a reason for hiding this comment

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

AM \ PM toggle changes selection to invalid time.

amPmToggleSelectsInvalidTime

Copy link
Copy Markdown
Member

@Derranion Derranion Jun 13, 2022

Choose a reason for hiding this comment

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

Still relevant

@rustem-nasyrov rustem-nasyrov requested a review from Derranion June 1, 2022 15:48
m0ksem
m0ksem approved these changes Jun 7, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@m0ksem m0ksem left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks much better than before

Derranion
Derranion reviewed Jun 8, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Derranion Derranion left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Checked this once more - still have these issues.

  • Selecting time at the bottom:

#1827 (comment)

  • Scrolling:

#1827 (comment)

  • AM \ PM toggle:

#1827 (comment)

@rustem-nasyrov rustem-nasyrov requested a review from Derranion June 14, 2022 08:45
Derranion
Derranion reviewed Jun 14, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Derranion Derranion left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We decided to move back a bit and use one modelValue for all Timepickers in the demo and make them work.

It means fixing these issues:

Checked this once more - still have these issues.

  • Selecting time at the bottom:

#1827 (comment)

  • Scrolling:

#1827 (comment)

  • AM \ PM toggle:

#1827 (comment)

@m0ksem m0ksem modified the milestones: 1.4.1, 1.4.3 Jun 23, 2022
@rustem-nasyrov rustem-nasyrov requested a review from Derranion June 30, 2022 14:12
@rustem-nasyrov
Copy link
Copy Markdown
Contributor Author

rustem-nasyrov commented Jun 30, 2022

After discussion with @Derranion, we decided to merge PR and create a new issue with am/pm flags.

@rustem-nasyrov rustem-nasyrov merged commit ee3d7df into epicmaxco:develop Jun 30, 2022
@rustem-nasyrov rustem-nasyrov deleted the feat/improve-time-picker-behavior branch June 30, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Derranion Derranion Derranion approved these changes

@m0ksem m0ksem m0ksem approved these changes

@RVitaly1978 RVitaly1978 RVitaly1978 approved these changes

@asvae asvae Awaiting requested review from asvae

+1 more reviewer

@aluarius aluarius aluarius approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rustem-nasyrov rustem-nasyrov

Labels

feature Something useful to end user

Projects

None yet

Milestone

1.4.5

Development

Successfully merging this pull request may close these issues.

TimePicker centered and auto selected time

6 participants

@rustem-nasyrov @asvae @Derranion @m0ksem @RVitaly1978 @aluarius