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

Fix accordion "jump" #32251

Closed
wants to merge 8 commits into from
Closed

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Nov 24, 2020

open accordion button has a border of 1px, but a closed one suppresses the border-bottom-width:0. this caused an odd 1px jump on the accordion panel. now, the border width is transitioned at the same pace as the panel opening/closing transition, so no more perceptible jump

Closes #32250

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

@patrickhlauke patrickhlauke requested a review from a team as a code owner November 24, 2020 23:15
@patrickhlauke patrickhlauke added this to Inbox in v4.6.0 via automation Nov 24, 2020
@patrickhlauke patrickhlauke added this to Inbox in v5.0.0-beta1 via automation Nov 24, 2020
open accordion button has a border of 1px, but a closed one suppresses the `border-bottom-width:0`. this caused an odd 1px jump on the accordion panel. now, the border width is transitioned at the same pace as the panel opening/closing transition, so no more perceptible jump
@patrickhlauke patrickhlauke removed this from Inbox in v4.6.0 Nov 24, 2020
@patrickhlauke
Copy link
Member Author

as https://getbootstrap.com/docs/4.5/components/collapse/#accordion-example doesn't seem to suffer from the same problem, this is v5 only

@XhmikosR
Copy link
Member

I still notice a tiny jump on my machine :/

@patrickhlauke
Copy link
Member Author

ah, i was fixated on the bump i was seeing on the panel itself, but seems the overall accordion as a whole also changes height by 1px overall of course due to that one 1px border of the one opened accordion. at this stage, no clue yet on how to tackle this...

@ffoodd
Copy link
Member

ffoodd commented Nov 25, 2020

@patrickhlauke Could using a transparent border instead of none be of any help here?

@ffoodd
Copy link
Member

ffoodd commented Nov 25, 2020

Basically:

.accordion-item:not(:last-of-type) .accordion-button.collapse {
  border-bottom-color: transparent;
}

Instead of:

.accordion-button.collapsed {
    border-bottom-width: 0;
}

Since there shouldn't be any height changes, transition should feel better.

@patrickhlauke
Copy link
Member Author

@ffoodd ouais, j'aime bien ça 👍

will give it a try this morning

@ffoodd
Copy link
Member

ffoodd commented Nov 25, 2020

Just edited my suggestion, made a mistake! Merci 😉

@patrickhlauke
Copy link
Member Author

unfortunately, that still doesn't quite solve it (and now for some reason the last accordion is missing the bottom border when collapsed)

think i'm going to leave this as is now until @MartijnCuppens has a chance to look it over ... feel free to push commits to this directly. also, the issue is easier to see when you zoom in, just for reference

@patrickhlauke patrickhlauke marked this pull request as draft November 25, 2020 12:42
@patrickhlauke
Copy link
Member Author

setting to draft until we properly worked this out...sorry, thought it was going to be easier, but my CSS-fu is letting me down

@XhmikosR XhmikosR removed this from Inbox in v5.0.0-beta1 Nov 25, 2020
@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta2 via automation Nov 25, 2020
@patrickhlauke patrickhlauke changed the title Add border to transitioned properties of accordion-button Fix accordion "jump" Nov 27, 2020
@@ -1028,7 +1028,7 @@ $accordion-button-padding-y: $accordion-padding-y !default;
$accordion-button-padding-x: $accordion-padding-x !default;
$accordion-button-color: $accordion-color !default;
$accordion-button-bg: $accordion-bg !default;
$accordion-transition: $btn-transition, border-radius .15s ease !default;
$accordion-transition: $btn-transition, border-radius .15s ease, border .35s ease !default; // border transition matches speed of transition-collapse
Copy link
Member

Choose a reason for hiding this comment

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

I think border transition here should be dropped or tweaked since border-color is already in transitioned in $btn-transition with .15s.

That may be causing the jump.

I tried a fw things without seeing anything so I'm not quite sure about this, but maybe using $btn-transition overlaps with this custom border transition.

Copy link
Member Author

Choose a reason for hiding this comment

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

well adding this was needed initially to fix the issue when the border was not transparent and going from a width of 1 to a width of 0. the issue may have been the fact that it's transitioning at .15s, but the height was transitioning at .35s, leading to one perceptible jump

Copy link
Member Author

Choose a reason for hiding this comment

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

now that it's just a colour change that particular part is likely not causing a jump, but there's some fundamental other jump going on somewhere for the accordion as a whole now (i.e. it's not the panel content itself that's jumping visibly anymore, but everything else is...)

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.

Thinking transition might be wrong. Keeping the transparent border should still help to ensure the height, but transition definetely needs some tweak

@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta3 via automation Dec 14, 2020
@XhmikosR XhmikosR removed this from Inbox in v5.0.0-beta2 Dec 14, 2020
@XhmikosR
Copy link
Member

@mdo: we also have #32278, not sure what's better but both solutions seem to have issues.

@XhmikosR XhmikosR moved this from Inbox to Review in v5.0.0-beta3 Feb 9, 2021
@mdo
Copy link
Member

mdo commented Feb 11, 2021

Compared to #32278, this is the cleaner fix for me when viewing in Chrome on my Windows machine. Going to close that one out and see if there's anything else we need to do to fix this up. As it stands now, this does appear like a fix for me.

@XhmikosR
Copy link
Member

There's definitely still a bump on my machine with Windows 10, 175% DPI, FF/Edgium.

@patrickhlauke
Copy link
Member Author

yeah i'm seeing a tiny bump still as well, and for the life of me can't quite work out why. wonder if this is coming down to weirdness related to hardware acceleration kicking in for the transition which is very subtly fuzzing up font metrics when it runs, or something

@XhmikosR
Copy link
Member

@mdo maybe you have an idea how to really fix this?

@mdo
Copy link
Member

mdo commented Feb 19, 2021

See #33149 for a potential fix!

@mdo
Copy link
Member

mdo commented Mar 2, 2021

Closing for #33149

@mdo mdo closed this Mar 2, 2021
@mdo mdo deleted the patrickhlauke-fix-accordion-jump branch March 2, 2021 17:12
@mdo mdo removed this from Review in v5.0.0-beta3 Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accordion slight jump when toggling items
4 participants