-
Notifications
You must be signed in to change notification settings - Fork 236
docs(color-handle): enhance README with detailed component overview #5663
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
Changes from 1 commit
36522c6
00b5b62
189b75e
f3ef3a0
6726c92
e7e43b5
ffb0126
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,45 +1,222 @@ | ||||||||||||||||||||||
| ## Description | ||||||||||||||||||||||
| ## Overview | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| The `<sp-color-handle>` is used to select a colour on an `<sp-color-area>`, `<sp-color-slider>`, or `<sp-color-wheel>`. It functions similarly to the handle on an `<sp-slider>`. | ||||||||||||||||||||||
| The `<sp-color-handle>` is used to select a color on an `<sp-color-area>`, `<sp-color-slider>`, or `<sp-color-wheel>`. It functions similarly to the handle on an `<sp-slider>`, providing a draggable control point for precise color selection within color components. | ||||||||||||||||||||||
|
||||||||||||||||||||||
| The `<sp-color-handle>` is used to select a color on an `<sp-color-area>`, `<sp-color-slider>`, or `<sp-color-wheel>`. It functions similarly to the handle on an `<sp-slider>`, providing a draggable control point for precise color selection within color components. | |
| The color handle is used to select a color on a [color area](../color-area/), [color slider](../color-slider/), or [color wheel](../color-wheel/). It provides a draggable control point for precise color selection within color components. |
blunteshwar marked this conversation as resolved.
Show resolved
Hide resolved
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.
| - A visual handle element that indicates the current position | |
| - An optional color loupe that appears above the handle when active | |
| - Touch-responsive interaction areas | |
| - Color display showing the current selected color | |
| - Opacity checkerboard pattern for transparent colors | |
| A visual handle element that indicates the current position | |
| - Touch-responsive interaction areas | |
| - Color display showing the current selected color | |
| - Opacity checkerboard pattern for transparent colors | |
| - An optional color loupe that appears above the handle when active |
Outdated
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.
this call out can be removed with the suggestion above, just added these details to the bullet points
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.
done
blunteshwar marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
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 should add a table of the colors accepted OR we should add a line about how color uses the color-controller and link to the color-controller docs with the table
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.
Done
Outdated
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.
Move this above the example
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.
I dont think this is necessary...? @nikkimk are we showing default states in all components?
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.
I include the default if I used tabs for a visual comparison, and typically for variants more than states, but it's not necessarily a hill to die on for me.
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.
I think adding a default let the users compare what is the difference b/w default and other states
Outdated
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 should show an example of how to set up the other input types
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.
This shouldnt be needed, all details should be covered in the first paragraph, make updates there if you feel the wording needs updating
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.
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 are not exposing this to users The streamingListener for internal use only
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.
but isnt the streamingListener how a consumer using handle to make a new component would set it up for the event handlers? there is not functional code in handle that sets them up
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.
since this is handled in the parent, should this example show more of the implementation pattern versus the output in the DOM?
cc @nikkimk
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.
I think it's good to include to show what focused state looks like and how it works within the component.
Outdated
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.
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.
Done!
Outdated
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.
Can we add a small example to surface this usage to our customers?
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.
Done!
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.
instead of an example here bringing in another component, I would like to suggest that we link to the other components docs to see it in use
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.
Done!
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.
What is the size of the touch target?
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.
The size of sp-color-loupe


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.
Can we highlight the functional difference between the color-handle and the handle in sp-slider? If there is no difference, I would remove the reference to sp-slider, as that causes confusion.
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.
Should we highlight that this is an atomic level component or maybe discuss the language we would like to use for internal/dumb components?
cc @nikkimk
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.
Done!
I added a note
**Note**:is a primitive component designed to be used within other color selection components. It's not typically used directly in applications, but rather as part of higher-level color components like,, or.