Skip to content

fix(tabs): selection indicator scroll overflow border #1513

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 6, 2022
Merged

fix(tabs): selection indicator scroll overflow border #1513

merged 1 commit into from
Oct 6, 2022

Conversation

yosevu
Copy link
Contributor

@yosevu yosevu commented Sep 12, 2022 •
edited
Loading

Description

Resolves an issue with the selection indicator border when there is scrollable overflow.

Updated with the recommended solution using the background linear gradient rather than background-color.

How and where has this been tested?

Set .spectrum-Tabs width to less than the widths of its .spectrum-Tab-item children combined e.g. 200px. Set .spectrum-Tabs to overflow: auto. See examples of the bug and fix below.

Bug

tabs-overflow-border-indicator-bug.mp4

Fixed

tabs-overflow-border-indicator.mp4

Screenshots

To-do list

  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have tested these changes in Windows High Contrast mode.
  • This pull request is ready to merge.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 12, 2022 •
edited
Loading

🚀 Deployed on https://pr-1513--spectrum-css.netlify.app

@github-actions github-actions bot temporarily deployed to pull request September 12, 2022 13:45 Inactive
yosevu
yosevu commented Sep 12, 2022
View reviewed changes
components/tabs/skin.css Outdated
var(--spectrum-tabs-textonly-tabitem-selection-indicator-background-color-selected) 0,
var(--spectrum-tabs-textonly-tabitem-selection-indicator-background-color-selected)
var(--spectrum-tabs-quiet-textonly-divider-size),
transparent var(--spectrum-tabs-quiet-textonly-divider-size)
Copy link
Contributor Author

@yosevu yosevu Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What token should I use in place of --spectrum-tabs-quiet-textonly-divider-size?

The widths are currently set as an inline styles, so I'm not sure the purpose of this token, which is only 2px:

Screen Shot 2022-09-12 at 9 02 08 AM

Screen Shot 2022-09-12 at 9 05 21 AM

Copy link
Collaborator

Choose a reason for hiding this comment

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

With regard to this specific usage, it looks like --spectrum-tabs-quiet-textonly-divider-size has two purposes: to set the height of the divider (the bottom border) and to modify its bottom position placement (-2px).

The inline styles defined in the example are likely just for the docs example. In a real world application, the implementation would probably have some logic to handle width & the left position values.

Can you no longer use --spectrum-tabs-quiet-textonly-divider-size? If not, what if you were to just manually define it and set it to 2px?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Patrick, I misunderstood how the width is was supposed to work and you helped clarify that.

I updated the PR and it looks correct now. Can I reproduce the issue in the the PR link to make sure this works as expected?

@yosevu yosevu force-pushed the yosevu/css-242-tabs-border-for-scrolling branch from 47e25b9 to fe866b9 Compare September 12, 2022 16:54
@github-actions github-actions bot temporarily deployed to pull request September 12, 2022 16:59 Inactive
@yosevu yosevu force-pushed the yosevu/css-242-tabs-border-for-scrolling branch 2 times, most recently from 89c854e to e8cfb22 Compare September 12, 2022 17:34
@github-actions github-actions bot temporarily deployed to pull request September 12, 2022 17:38 Inactive
@yosevu yosevu force-pushed the yosevu/css-242-tabs-border-for-scrolling branch from e8cfb22 to 1916e30 Compare September 12, 2022 17:51
@github-actions github-actions bot temporarily deployed to pull request September 12, 2022 17:57 Inactive
@yosevu yosevu force-pushed the yosevu/css-242-tabs-border-for-scrolling branch from 1916e30 to 05d9073 Compare September 12, 2022 17:57
@github-actions github-actions bot temporarily deployed to pull request September 12, 2022 18:02 Inactive
@yosevu yosevu force-pushed the yosevu/css-242-tabs-border-for-scrolling branch from 05d9073 to f575e3e Compare October 5, 2022 07:03
@github-actions github-actions bot temporarily deployed to pull request October 5, 2022 07:06 Inactive
@yosevu yosevu requested a review from pfulton October 5, 2022 11:18
pfulton
pfulton approved these changes Oct 5, 2022
View reviewed changes
Copy link
Collaborator

@pfulton pfulton left a comment

Choose a reason for hiding this comment

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

Awesome. Looks great to me. Thanks for taking care of this.

@lunarfusion
Copy link
Contributor

I submitted a question in Slack about the design intent of this component to make sure this change isn't misaligned with the design. Originally the bottom border and selection indicator fell outside the block size of the component, leading to the overflow issue. This PR fixes the problem, but it also changes the design. Below is the before (left) and after (right).
Screen Shot 2022-10-05 at 11 45 18 AM

@yosevu
Copy link
Contributor Author

yosevu commented Oct 5, 2022

I submitted a question in Slack about the design intent of this component to make sure this change isn't misaligned with the design. Originally the bottom border and selection indicator fell outside the block size of the component, leading to the overflow issue. This PR fixes the problem, but it also changes the design. Below is the before (left) and after (right). Screen Shot 2022-10-05 at 11 45 18 AM

PJ looked at this deploy and confirmed that the indicator is correctly within the Tabs height.

@pfulton
fix(tabs): selection indicator scroll overflow border
251022f 
CSS-242

fix: update solution

fix: update vertical variant

Still need to update indicator colors and quiet variants

fix: all variants correct

chore: remove comments
@pfulton pfulton force-pushed the yosevu/css-242-tabs-border-for-scrolling branch from f575e3e to 251022f Compare October 6, 2022 20:50
@github-actions github-actions bot temporarily deployed to pull request October 6, 2022 20:53 Inactive
@pfulton pfulton merged commit 3f740ad into main Oct 6, 2022
@pfulton pfulton deleted the yosevu/css-242-tabs-border-for-scrolling branch October 6, 2022 20:55
bernhard-adobe added a commit that referenced this pull request Oct 12, 2022
@bernhard-adobe
chore: tokens 3.0.0 and merge branch 'main'
6dfc155 
* main: (53 commits)
  chore(release): release
  fix(checkbox): whcm focus states (#1527)
  chore(release): release
  feat(fieldlabel)!: migrate to core tokens (CSS-102) (#1476)
  chore(release): release
  feat(swatchgroup)!: migrate swatchgroup to core tokens (#1505)
  chore(release): release
  feat(swatch)!: migrate swatch to core tokens (#1501)
  chore(release): release
  fix(tabs): selection indicator scroll overflow border (#1513)
  chore(release): release
  feat(divider)!: migrate to core tokens
  chore(release): release
  refactor(checkbox): remove commented out code (#1524)
  chore(release): release
  feat(progresscircle)!: migrate to core tokens
  chore(release): release
  feat(checkbox)!: migrate checkbox component to core tokens (CSS-99) (#1465)
  chore(release): release
  fix(card): increase content area height when necessary
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pfulton pfulton pfulton approved these changes

@lunarfusion lunarfusion Awaiting requested review from lunarfusion

@bernhard-adobe bernhard-adobe Awaiting requested review from bernhard-adobe

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yosevu @lunarfusion @pfulton