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

Revamp accordion borders to fix pixel jumping #33149

Merged
merged 3 commits into from
Mar 17, 2021
Merged

Revamp accordion borders to fix pixel jumping #33149

merged 3 commits into from
Mar 17, 2021

Conversation

mdo
Copy link
Member

@mdo mdo commented Feb 19, 2021

This should fix the issue once and for all—this takes a simpler approach to the borders and seems to fix all the movement I could see (which was actually visible without even zooming in, at least in Safari).

  • Pushes the borders to the .accordion-item instead of the .accordion-button
  • Simplifies the .accordion-flush's border resets
  • Recreates the .accordion-button's border-bottom with an inset box-shadow
  • Moves the border-radius to the parent, which necessitated the new $accordion-inner-border-radius variable

Fixes #32250, closes #32251.

Preview: https://deploy-preview-33149--twbs-bootstrap.netlify.app/docs/5.0/components/accordion/

@XhmikosR
Copy link
Member

XhmikosR commented Feb 19, 2021

Definitely better from a quick look!

Using Firefox on Windows 10 with 175% DPI: it seems there's some new issues, like double border in the first item, missing border-bottom is the other items, and a slight border-radius issue in the last item

image

@mdo
Copy link
Member Author

mdo commented Feb 26, 2021

Pushed an update that I hope will address the FF zoom issue.

@mdo mdo force-pushed the accordion-schmordion branch 3 times, most recently from 465dc4e to 9b14981 Compare March 2, 2021 15:42
@mdo
Copy link
Member Author

mdo commented Mar 4, 2021

@ffoodd Any chance you could give me a quick review here? <3

@ffoodd
Copy link
Member

ffoodd commented Mar 4, 2021

I still see a little glitch but it's better.

FF zoom issue persists (I start to see weirdness from 110% to 170%, either a missing border or doubled ones).

I guess our main issue relates more to animating height than to borders. Shouldn't we try something based on visibility & max-height instead? My main reference on the topic is this van11y hide/show plugin (check the "Animation" section in "Bonuses").

Not quite sure this is the way to go, though…

@mdo
Copy link
Member Author

mdo commented Mar 16, 2021

@ffoodd I think we merge this as-is and then look at revamping the animations separately. Thoughts?

@ffoodd
Copy link
Member

ffoodd commented Mar 16, 2021

Agreed, :shipit:

Should probably open a new issue dedicated to animation though.

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

Only left a question. Simpler and better solution IMHO, let's do this.

scss/_accordion.scss Outdated Show resolved Hide resolved
mdo added 2 commits March 16, 2021 21:33
- Pushes the borders to the .accordion-item
- Simplifies the .accordion-flush
- Recreates the .accordion-button border-bottom with an inset box-shadow
- Updates background-color to solid to match other components like list group and prevent stacked translucent borders
- Use negative margin to overlap the consecutive items
@mdo mdo requested a review from a team as a code owner March 17, 2021 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accordion slight jump when toggling items
3 participants