[SegmentedControl] Refinements #1177
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…, remove horizontal padding
winstonkim
approved these changes
Sep 24, 2021
…so focus ring is consistent across control
winstonkim
added a commit
that referenced
this pull request
Sep 28, 2021
* add SegmentedControl element * add docs * add handling for active index * add type for example element * add variable items to example story * update comments * add tests * fix styling * update snapshot * add focus box-shadow * update snapshot * remove Knobs * update * focus styling * update snapshot * return null if length is less than 1 * update * Update to new Component Story Format * Remove markdown file * Pull COLORS from theme object, use theme test utils, update onClick test * use React Testing Library instead of Enzyme * Add secondary stories * Update snapshots * Add sstyling for secondary theme * Run ESLint fix * Use BORDER_RADIUS.large (32px equivalent to 80px) * [SegmentedControl] Refinements (#1177) * Run ESLint fix * Use BORDER_RADIUS.large (32px equivalent to 80px) * Move border to items instead of container to account for absolutely positioned indicator * Update snapshots * Rename width to segmentWidth to not set width property to DOM element, remove horizontal padding * Remove errant (redundant?) box-shadow, use padding instead of border so focus ring is consistent across control * Add slight margin to get button dimensions better matching Figma file * Add regression tests for focus states * Update snapshot * use Z_SCALE constant * Update snapshot for 40px height change Co-authored-by: Michael Altamirano <michaeljaltamirano@gmail.com> Co-authored-by: Winston Kim <winzton.kim@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CHANGELOG
Run
eslint --fixto address new standards since branch was created.Replace
80pxborder-radiuswiththeme.BORDER_RADIUS.large.0for the secondary theme, and it is32pxfor the primary theme, which is equal to80px(you can edit in your dev tools to examine this.)Update
transition: transform 0.22stotransition: transform 0.3sto match300msspecified in Figma spec:Move
borderstyle from item container to individual segment items to prevent issue ofIndicatorbeing presentationally different from the other segment items.position: absoluteelement was larger because theborderon the parent was not affecting the height.TODO: There' still some refinement that could be made here: the absolutely positioned indicator is 172px width compared to 168px width for the non-indicator buttons, so they should be made more consistent, probably.
Update the width handling--by removing the horizontal padding from
padding: 0.25rem--such that theposition: absoluteIndicator element width matches the other elements.Indicator.position.before.mov
Indicator.position.after.mov
Add visual regression tests for un-selected focus state.
TODO for future PRs
The current functionality sits somewhere between a RadioGroup and a TabList. In both examples the keyboard controls use tabbing to focus the options, and then arrow keys to toggle between the options, where changing the selection is done automatically, without a button "click" event.
If we decide to use neither of these paradigms, the biggest problem right now is that because the Indicator comes before the more appropriately interactive button elements, toggling through the options sometimes paradoxically goes backwards in order. (See also this comment in the other PR.) This video shows normal tabbing behavior: the first selection is the indicator, and then it starts from the first available button, and then skips the button that matches the indicator selection and proceeds to the next focusable element:
segmentedcontrol.tab.behavior.mov
This needs to be fixed in a future PR, but for now we are maintaining this behavior to unblock UI development this week.