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

SliderControl: Create new control using imported G2 Slider component #42315

Closed
wants to merge 14 commits into from

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Jul 11, 2022

Related:

What?

Creates a new SliderControl component representing only the slider part of the RangeControl i.e. no number input field, no icons etc.

Why?

This is a step toward better, more modular slider-related controls.

How?

  • Uses imported G2 Slider component internally (this slider input is not exported)
  • Imports interpolate utils from G2
  • Follows current format recommended by guidelines e.g. TypeScript, Emotion, hooks/components etc.
  • Leverages BaseControl for labelling purposes etc.
  • SliderControl has a default size of 36px with an __unstable-large variant for the future 40px default.
  • Adds customization of the thumb, track foreground and track background colors.

Todo

  • Write unit tests
  • Add a component readme
  • Fix mouse and hover event handling
  • Fix custom tooltip positioning while at extremes of slider

Testing Instructions

TBA...

  • Fire up the Storybook and ensure the SliderControl functions appropriately
  • Compare the SliderControl stories with those for the RangeControl
  • Run unit tests for the Gutenberg Slider npm run test-unit packages/components/src/slider/test

Screenshots or screencast

Screen.Recording.2022-08-05.at.11.48.06.am.mp4

@aaronrobertshaw aaronrobertshaw added the [Feature] UI Components Impacts or related to the UI component system label Jul 11, 2022
@github-actions
Copy link

github-actions bot commented Jul 11, 2022

Size Change: +27 B (0%)

Total Size: 1.27 MB

Filename Size Change
build/components/index.min.js 199 kB +27 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 982 B
build/annotations/index.min.js 2.76 kB
build/api-fetch/index.min.js 2.26 kB
build/autop/index.min.js 2.14 kB
build/blob/index.min.js 475 B
build/block-directory/index.min.js 7.09 kB
build/block-directory/style-rtl.css 990 B
build/block-directory/style.css 991 B
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-editor/index.min.js 167 kB
build/block-editor/style-rtl.css 15.4 kB
build/block-editor/style.css 15.4 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/audio/theme-rtl.css 126 B
build/block-library/blocks/audio/theme.css 126 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 84 B
build/block-library/blocks/avatar/style.css 84 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 482 B
build/block-library/blocks/button/editor.css 482 B
build/block-library/blocks/button/style-rtl.css 523 B
build/block-library/blocks/button/style.css 523 B
build/block-library/blocks/buttons/editor-rtl.css 337 B
build/block-library/blocks/buttons/editor.css 337 B
build/block-library/blocks/buttons/style-rtl.css 332 B
build/block-library/blocks/buttons/style.css 332 B
build/block-library/blocks/calendar/style-rtl.css 239 B
build/block-library/blocks/calendar/style.css 239 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 100 B
build/block-library/blocks/categories/style.css 100 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/style-rtl.css 121 B
build/block-library/blocks/code/style.css 121 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 406 B
build/block-library/blocks/columns/style.css 406 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 187 B
build/block-library/blocks/comment-template/style.css 185 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 834 B
build/block-library/blocks/comments/editor.css 832 B
build/block-library/blocks/comments/style-rtl.css 632 B
build/block-library/blocks/comments/style.css 630 B
build/block-library/blocks/cover/editor-rtl.css 605 B
build/block-library/blocks/cover/editor.css 607 B
build/block-library/blocks/cover/style-rtl.css 1.57 kB
build/block-library/blocks/cover/style.css 1.55 kB
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/embed/theme-rtl.css 126 B
build/block-library/blocks/embed/theme.css 126 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 253 B
build/block-library/blocks/file/style.css 254 B
build/block-library/blocks/file/view.min.js 346 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 948 B
build/block-library/blocks/gallery/editor.css 950 B
build/block-library/blocks/gallery/style-rtl.css 1.53 kB
build/block-library/blocks/gallery/style.css 1.53 kB
build/block-library/blocks/gallery/theme-rtl.css 108 B
build/block-library/blocks/gallery/theme.css 108 B
build/block-library/blocks/group/editor-rtl.css 384 B
build/block-library/blocks/group/editor.css 384 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 76 B
build/block-library/blocks/heading/style.css 76 B
build/block-library/blocks/html/editor-rtl.css 327 B
build/block-library/blocks/html/editor.css 329 B
build/block-library/blocks/image/editor-rtl.css 884 B
build/block-library/blocks/image/editor.css 882 B
build/block-library/blocks/image/style-rtl.css 627 B
build/block-library/blocks/image/style.css 630 B
build/block-library/blocks/image/theme-rtl.css 126 B
build/block-library/blocks/image/theme.css 126 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 463 B
build/block-library/blocks/latest-posts/style.css 462 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 507 B
build/block-library/blocks/media-text/style.css 505 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 705 B
build/block-library/blocks/navigation-link/editor.css 703 B
build/block-library/blocks/navigation-link/style-rtl.css 115 B
build/block-library/blocks/navigation-link/style.css 115 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 296 B
build/block-library/blocks/navigation-submenu/editor.css 295 B
build/block-library/blocks/navigation/editor-rtl.css 2.01 kB
build/block-library/blocks/navigation/editor.css 2.02 kB
build/block-library/blocks/navigation/style-rtl.css 2.17 kB
build/block-library/blocks/navigation/style.css 2.16 kB
build/block-library/blocks/navigation/view-modal.min.js 2.78 kB
build/block-library/blocks/navigation/view.min.js 443 B
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 363 B
build/block-library/blocks/page-list/editor.css 363 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 317 B
build/block-library/blocks/paragraph/editor.css 317 B
build/block-library/blocks/paragraph/style-rtl.css 279 B
build/block-library/blocks/paragraph/style.css 281 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/editor-rtl.css 96 B
build/block-library/blocks/post-comments-form/editor.css 96 B
build/block-library/blocks/post-comments-form/style-rtl.css 493 B
build/block-library/blocks/post-comments-form/style.css 493 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 586 B
build/block-library/blocks/post-featured-image/editor.css 584 B
build/block-library/blocks/post-featured-image/style-rtl.css 315 B
build/block-library/blocks/post-featured-image/style.css 315 B
build/block-library/blocks/post-navigation-link/style-rtl.css 153 B
build/block-library/blocks/post-navigation-link/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 282 B
build/block-library/blocks/post-template/style.css 282 B
build/block-library/blocks/post-terms/style-rtl.css 96 B
build/block-library/blocks/post-terms/style.css 96 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 326 B
build/block-library/blocks/pullquote/style.css 325 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 282 B
build/block-library/blocks/query-pagination/style.css 278 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 439 B
build/block-library/blocks/query/editor.css 439 B
build/block-library/blocks/quote/style-rtl.css 213 B
build/block-library/blocks/quote/style.css 213 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 409 B
build/block-library/blocks/search/style.css 406 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 234 B
build/block-library/blocks/separator/style.css 234 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 464 B
build/block-library/blocks/shortcode/editor.css 464 B
build/block-library/blocks/site-logo/editor-rtl.css 488 B
build/block-library/blocks/site-logo/editor.css 488 B
build/block-library/blocks/site-logo/style-rtl.css 203 B
build/block-library/blocks/site-logo/style.css 203 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 674 B
build/block-library/blocks/social-links/editor.css 673 B
build/block-library/blocks/social-links/style-rtl.css 1.4 kB
build/block-library/blocks/social-links/style.css 1.39 kB
build/block-library/blocks/spacer/editor-rtl.css 322 B
build/block-library/blocks/spacer/editor.css 322 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 494 B
build/block-library/blocks/table/editor.css 494 B
build/block-library/blocks/table/style-rtl.css 611 B
build/block-library/blocks/table/style.css 609 B
build/block-library/blocks/table/theme-rtl.css 190 B
build/block-library/blocks/table/theme.css 190 B
build/block-library/blocks/tag-cloud/style-rtl.css 239 B
build/block-library/blocks/tag-cloud/style.css 239 B
build/block-library/blocks/template-part/editor-rtl.css 235 B
build/block-library/blocks/template-part/editor.css 235 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 691 B
build/block-library/blocks/video/editor.css 694 B
build/block-library/blocks/video/style-rtl.css 174 B
build/block-library/blocks/video/style.css 174 B
build/block-library/blocks/video/theme-rtl.css 126 B
build/block-library/blocks/video/theme.css 126 B
build/block-library/classic-rtl.css 162 B
build/block-library/classic.css 162 B
build/block-library/common-rtl.css 1.02 kB
build/block-library/common.css 1.02 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/editor-rtl.css 11.2 kB
build/block-library/editor.css 11.2 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/index.min.js 191 kB
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/style-rtl.css 12.3 kB
build/block-library/style.css 12.3 kB
build/block-library/theme-rtl.css 719 B
build/block-library/theme.css 722 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/blocks/index.min.js 49.8 kB
build/components/style-rtl.css 11.2 kB
build/components/style.css 11.2 kB
build/compose/index.min.js 12.5 kB
build/core-data/index.min.js 15.5 kB
build/customize-widgets/index.min.js 11.3 kB
build/customize-widgets/style-rtl.css 1.38 kB
build/customize-widgets/style.css 1.38 kB
build/data-controls/index.min.js 653 B
build/data/index.min.js 8.08 kB
build/date/index.min.js 32.1 kB
build/deprecated/index.min.js 507 B
build/dom-ready/index.min.js 324 B
build/dom/index.min.js 4.7 kB
build/edit-navigation/index.min.js 16 kB
build/edit-navigation/style-rtl.css 3.99 kB
build/edit-navigation/style.css 4 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/edit-post/index.min.js 31.1 kB
build/edit-post/style-rtl.css 6.94 kB
build/edit-post/style.css 6.94 kB
build/edit-site/index.min.js 57.9 kB
build/edit-site/style-rtl.css 8.36 kB
build/edit-site/style.css 8.34 kB
build/edit-widgets/index.min.js 16.5 kB
build/edit-widgets/style-rtl.css 4.34 kB
build/edit-widgets/style.css 4.34 kB
build/editor/index.min.js 41.6 kB
build/editor/style-rtl.css 3.62 kB
build/editor/style.css 3.61 kB
build/element/index.min.js 4.68 kB
build/escape-html/index.min.js 537 B
build/experiments/index.min.js 868 B
build/format-library/index.min.js 6.77 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.64 kB
build/html-entities/index.min.js 448 B
build/i18n/index.min.js 3.77 kB
build/is-shallow-equal/index.min.js 527 B
build/keyboard-shortcuts/index.min.js 1.78 kB
build/keycodes/index.min.js 1.83 kB
build/list-reusable-blocks/index.min.js 2.13 kB
build/list-reusable-blocks/style-rtl.css 835 B
build/list-reusable-blocks/style.css 835 B
build/media-utils/index.min.js 2.93 kB
build/notices/index.min.js 963 B
build/nux/index.min.js 2.06 kB
build/nux/style-rtl.css 732 B
build/nux/style.css 728 B
build/plugins/index.min.js 1.94 kB
build/preferences-persistence/index.min.js 2.22 kB
build/preferences/index.min.js 1.3 kB
build/primitives/index.min.js 933 B
build/priority-queue/index.min.js 1.58 kB
build/react-i18n/index.min.js 696 B
build/react-refresh-entry/index.min.js 8.44 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.74 kB
build/reusable-blocks/index.min.js 2.21 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 10.6 kB
build/server-side-render/index.min.js 1.61 kB
build/shortcode/index.min.js 1.53 kB
build/style-engine/index.min.js 1.46 kB
build/token-list/index.min.js 644 B
build/url/index.min.js 3.61 kB
build/vendors/react-dom.min.js 38.5 kB
build/vendors/react.min.js 4.34 kB
build/viewport/index.min.js 1.08 kB
build/warning/index.min.js 268 B
build/widgets/index.min.js 7.18 kB
build/widgets/style-rtl.css 1.18 kB
build/widgets/style.css 1.19 kB
build/wordcount/index.min.js 1.06 kB

compressed-size-action

@aaronrobertshaw aaronrobertshaw self-assigned this Jul 18, 2022
@aaronrobertshaw aaronrobertshaw changed the title [WIP] Slider: Import G2 Slider component and required utils Slider: Import G2 Slider component and required utils Jul 18, 2022
@aaronrobertshaw aaronrobertshaw marked this pull request as ready for review July 18, 2022 08:50
@aaronrobertshaw
Copy link
Contributor Author

From #40507

Create a new SliderControl component. This component would basically be only the slider part of RangeControl (ie. no number input field, no icons..).

When it comes to the implementation of this component, we have a couple of choices: we could use the code from RangeControl, or instead use the approach from the g2 Slider. In both cases, we should then "adapt" the code to the current format recommended by the guidelines (e.g. TypeScript, Emotion, hooks/components..)

I'm currently working on the new SliderControl component mentioned above but figured it might not be too early to get some eyes on the import of the G2 Slider. Hopefully, it isn't too far off the mark 🤞

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Hey @aaronrobertshaw , thank you for working on this! I started having a preliminary look and left a few comments around.

One thing that came to mind is if this component should be wrapped with BaseControl, which would add support for label and help text

From what I can see, the component is already looking quite good! We may still need to implement a few features from RangeSlider :

  • marks
  • step prop
  • labels and help text
  • resetFallbackValue (although we may argue it may be a bit redundant)
  • tooltip support

packages/components/src/slider/slider/hook.ts Outdated Show resolved Hide resolved
let next = `${ nextValue }`;

if ( initialUnit ) {
next = createCSSUnitValue( nextValue, initialUnit );
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't slider only deal with unitless values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The G2 Slider dealt with units so I maintained that approach here.

Only dealing with unitless values would simplify this component in favour of moving that logic to any other component that uses the Slider and units in its values. Is there value in keeping that logic here so it is consistent for all consumers of Slider?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my views, SliderControl should deal with unitless values.

Any other component that components SliderControl together with other input controls dealing with units (e.g UnitControl) will have to keep the numeric part in sync with SliderControl internally (similarly to what UnitControl does internally with NumberControl)

packages/components/src/slider/slider/hook.ts Outdated Show resolved Hide resolved
Comment on lines 103 to 104
isFocused && styles.focused( colors ),
error && isFocused && styles.focusedError,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use CSS pseudo-selectors instead of JS logic for focused and focused error states?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did question this initially but ended up importing this faithfully from the G2 implementation of the Slider.

Today, I've dug deeper into why this approach was taken. In Safari, I ran into problems getting it to reflect the focus state when solely relying on pseudo-selectors.

It appears that there is something funky with its shadow DOM for the range input. If you toggle on the :focus state for the input via Safari's dev tools, you'll see the correct focus styling. Simply interacting with the slider, however, doesn't seem to result in the input matching the focused state.

This JS logic does achieve the consistent focus styling for me across browsers.

} );
const [ value, initialUnit ] = parseCSSUnitValue( `${ _value }` );

const id = useFormGroupContextId( idProp );
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this used across the package for all control related components? If so, can it be promoted outside of the ui folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The FormGroup readme say it's experimental at the moment. I'm not sure if it is ready for wider use but wanted to stay as close to the G2 version as possible using what was already available. Would this be better addressed in a follow-up?

Copy link
Contributor

Choose a reason for hiding this comment

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

I gave FormControl a deeper look — it seems like it partially overlaps with BaseControl.

I would probably switch to BaseControl for the time being, and later have a look at FormGroup and at the chance of consolidating BaseControl and FormGroup into one component (this may be also interesting / useful in the context of the recent challenge of labelling compound components that we're facing with BorderBoxControl)

packages/components/src/slider/slider/hook.ts Outdated Show resolved Hide resolved
packages/components/src/slider/styles.ts Outdated Show resolved Hide resolved
packages/components/src/slider/styles.ts Outdated Show resolved Hide resolved
packages/components/src/utils/config-values.js Outdated Show resolved Hide resolved
*
* @return {number} The interpolated value.
*/
export function interpolate(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add unit tests for interpolate and interpolateRounded ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll write some unit tests for these and have them available early next week. I've run out of time for today.

@aaronrobertshaw
Copy link
Contributor Author

Thanks for the early feedback @ciampo, it's always super helpful 🙇

One thing that came to mind is if this component should be wrapped with BaseControl, which would add support for label and help text
...
We may still need to implement a few features from RangeSlider

My apologies for not being clearer in the PR description or my earlier comment. When I noted the intent of this component to be a "low-level internal slider input", it was that this component would encapsulate just the input[type="range"] itself. I envisioned a SliderControl component adding labelling, marks, tooltips, help text etc around the Slider making it a real control.

Another part of my thinking was keeping this Slider more comparable to the G2 Slider in terms of functionality would make the review process easier and keep PRs to a more manageable size.

A further breakdown of how I saw these slider-related controls is:

Slider

  • This is the slider input itself i.e. input[type="range"]

SliderControl

  • The main wrapper around the Slider input
  • Provides the marks, tooltips, icons etc
  • Also, accepts children which can be rendered on the same line as the input. Think NumberControl and reset Button as per RangeControl

SliderNumberControl

  • Aims to satisfy the requirements of a RangeControl
  • Essentially is just a convenience component that is a SliderControl with optional NumberControl and reset Button as children
  • Has all the other internals you'd expect, e.g. triggering change handlers on either input change, resetting etc.
  • This could possibly handle mapping any difference in RangeControl props to the new component's API to make aliasing RangeControl trivial

SliderUnitControl

  • Similar to the SliderNumberControl
  • Convenience component that is a SliderControl with optional UnitControl as a child.

I hope to have the SliderControl up in a PR based on this one in the next couple of days. Perhaps we can re-evaluate the general approach then unless you see any red flags now?

As for the individual feedback items in your last review, I'll work through them today.

@ciampo
Copy link
Contributor

ciampo commented Jul 21, 2022

Thank you for clarifying!

When I noted the intent of this component to be a "low-level internal slider input", it was that this component would encapsulate just the input[type="range"] itself. I envisioned a SliderControl component adding labelling, marks, tooltips, help text etc around the Slider making it a real control.

Another part of my thinking was keeping this Slider more comparable to the G2 Slider in terms of functionality would make the review process easier and keep PRs to a more manageable size.

A further breakdown of how I saw these slider-related controls is:

Slider

  • This is the slider input itself i.e. input[type="range"]

SliderControl

  • The main wrapper around the Slider input
  • Provides the marks, tooltips, icons etc
  • Also, accepts children which can be rendered on the same line as the input. Think NumberControl and reset Button as per RangeControl

After discussing with @mirka, and referring to the roadmap outlined in #40507, we don't really see the utility of having Slider as a separate component (i.e. living in its separate slider folder). At most, the Slider component could be a sub-component / implementation detail of the SliderControl component (but still not exported from the slider-control folder).

Also, we don't think that SliderControl should accept children to be rendered on "the same line". The SliderControl should basically just take care of displaying an "enhanced" slider input, implementing only the slider-related features from RangeControl, and not worrying about compound inputs or layouts.

The remaining features of RangeControl, like composing the slider input with other components (like icons, NumberControl, UnitControl..), should be tackled by higher-level components (like the proposed SliderNumberControl and SliderUnitControl).

As we will work on these higher-level components, we will have a chance at discussing a few aspects:

  • what should the internal layout options be? (e.g always display the components in a row? Or maybe on a grid? In which order? Can those be customised?)
  • there are a few props / features from RangeControl that feel a bit "extra", like icon props and reset button. Maybe we could decide to exclude them from SliderNumberControl and SliderUnitControl, and only include them in the compat-layer that we're going to add to "translate" from RangeControl to SliderNumberControl ?
  • how do we best label compound components SliderNumberControl and SliderUnitControl ? This is something that came up recently related to BorderControl and BorderBoxControl, but the same will apply to these components too

*
* @default 'default'
*/
size?: 'small' | 'default' | 'large';
Copy link
Member

@mirka mirka Jul 22, 2022

Choose a reason for hiding this comment

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

Ugh I'm having a hard time deciding what to do with sizes in this one. We can kind of start with a clean slate. I'm thinking default=36 and __unstable-large=40. Do you think we can get away with not adding a 30px variant at all? I'm hoping so, because all our basic components now have at least a 36px variant (#39397).

Also we can basically ignore the controlHeight* variables in CONFIG — those are not relevant anymore and are due for removal.

@aaronrobertshaw aaronrobertshaw changed the title Slider: Import G2 Slider component and required utils SliderControl: Create new control using imported G2 Slider component Aug 5, 2022
@aaronrobertshaw aaronrobertshaw added the [Package] Components /packages/components label Aug 5, 2022
@aaronrobertshaw aaronrobertshaw marked this pull request as draft August 5, 2022 02:05
@aaronrobertshaw aaronrobertshaw force-pushed the add/slider-component branch 2 times, most recently from d0b97ff to 82b8657 Compare September 23, 2022 08:02
@aaronrobertshaw
Copy link
Contributor Author

Just a quick update to note there is ongoing work to clean up the NumberControl types around the value prop that the new Slider controls would benefit from.

I'll dust off this PR and the follow-ups for the SliderNumberControl and SliderUnitControl once #45982 lands. This most likely will not be until the new year.

@ciampo
Copy link
Contributor

ciampo commented Jan 3, 2023

Absolutely — I've been AFK for the past month. I'll finish catching up and resume work on #45982 ASAP

@aaronrobertshaw
Copy link
Contributor Author

Going to close this PR for now.

It is still dependent on #45982 but can be resurrected if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components
Projects
Status: Abandoned ⛔️
Development

Successfully merging this pull request may close these issues.

3 participants