Skip to content

Conversation

@daniel-stoian-lgp
Copy link
Contributor

@daniel-stoian-lgp daniel-stoian-lgp commented Mar 12, 2021

Checklist

  • I have read and understand the contribution guide
  • A CHANGELOG entry is included
  • At least one test case is included for this feature or bug fix
  • Documentation was added or is not needed
  • Documentation was verified or is not changed
  • UI test was passed or is not needed
  • Screenshot test was verified or is not needed
  • This is an API breaking change

Issue Resolved / Feature Added

UX design for picker components required a drum/wheel behavior of picker components

Resolution

  • Added new internal/DrumPicker component
  • It has support for mouse/touch events to change increment/decrement the value. The list of options can be moved using mouseDown+MouseMove+MosueUp
  • Also supports changing the value using the 5-way navigation and on click the increment/decrement Area.
  • Styling for Silicon skin is not part of this implementation
  • When wrap is true, the value can be changed from last to first only via 5-way, for the moment

Additional Considerations

Links

PLAT-136771

Comments

Enact-DCO-1.0-Signed-off-by: Daniel Stoian daniel.stoian@lgepartner.com

@stanca-pop-lgp
Copy link
Contributor

OK then, LGTM :)

@jeonghee27
Copy link
Contributor

jeonghee27 commented Mar 31, 2021

We expected POC, but you've spent a lot of time because you implemented it all.
if there was a mis-communication, sorry.
The spec files do not have to be modified in this step. We only wanted to see the direction of the implementation of drumPicker.

Anyway, I checked the functions, not code.
the functions looks good, but we are worried about creating all DOM.
Hundreds of item DOMs are created in one Date Picker.
This can make performance problems.
Is there any way to reduce the number of DOM?
You can implement it to be not compatible with the existing Picker structure and can be implemented as a independent component. (Because it is POC)

@enactjs enactjs deleted a comment from stanca-pop-lgp Jun 3, 2021
Copy link
Member

@seunghoh seunghoh left a comment

Choose a reason for hiding this comment

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

Let's clean up this PR and revisit when we need to support in the Agate
I set request changes just in case click the merge accidentally.
Thanks for your hard work. This work will defiantly worth

@CLAassistant
Copy link

CLAassistant commented Jan 28, 2022

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants