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

Reset styles when popper is calculating position #32510

Closed
wants to merge 1 commit into from

Conversation

MartijnCuppens
Copy link
Member

@MartijnCuppens MartijnCuppens commented Dec 16, 2020

js/src/dropdown.js Outdated Show resolved Hide resolved
@MartijnCuppens MartijnCuppens force-pushed the main-mc-popperfix branch 3 times, most recently from 230a745 to 28ca7cb Compare December 17, 2020 08:12
@ffoodd
Copy link
Member

ffoodd commented Dec 17, 2020

I'm not sure why but the behaviour changed in RTL, translate() does not move horizontally anymore:

I can't see the relation with this PR though, but noticed it. Might be unrelated.

this._popper = Popper.createPopper(referenceElement, this._menu, this._getPopperConfig())
this._menu.classList.remove(CLASS_NAME_POPPER_ACTIVE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we remove the class again? When the element if positioned by popper the right property will not have an effect anyway or am I wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I would expect too, but it turns out this is needed by popper.

@rohit2sharma95
Copy link
Collaborator

rohit2sharma95 commented Dec 29, 2020

As per the comment written in the code here:

// Reset positioning when positioned with Popper
&[style] {
right: auto#{"/* rtl:ignore */"} !important; // stylelint-disable-line declaration-no-important
}
}

This right style was added to reset the position when the dropdown is positioned with Popper. (This was not in V4 where Popper V1 is used)
I tried to create a raw dropdown only with native Popper. And found that I don't need to reset the position when the element is placed by Popper.

As per my research, if you remove the right style from the dropdown, it works fine (even when Popper is not used): https://bootstrap-1jtqi8bem.vercel.app/docs/5.0/components/dropdowns/

Or maybe I am missing something @twbs/css-review 🙂

Edit: Noticed that initially .dropdown-menu-end is shown on the left side here, but if you scroll the window it is placed fine:
image

@septatrix
Copy link
Contributor

I tried to get rid of all the top/right/bottom/left styles which worker rather well except for the statically positioned examples (which do not use poppers dynamic positioning. It should work to only set these properties when [data-bs-display=static] matches or the element is inside a navbar (which are treated similar) however I do not have the time now to further experiment with this. Temporarily toggling the styles for popper however seems rather hacky (albeit appearing to work).

@rohit2sharma95
Copy link
Collaborator

Popper does not require the elements to have positioning styles and it can not place elements correctly that have positioning styles already.
https://codepen.io/rohit2sharma95/pen/wvzyGRM

It's not really something Popper aims to support, the requirement is that the target popper element should not be positioned already.

floating-ui/floating-ui#1206 (reply in thread)

@mdo
Copy link
Member

mdo commented Jan 8, 2021

Is this supposed to fix the dropdown-menu-end alignment at https://deploy-preview-32510--twbs-bootstrap.netlify.app/docs/5.0/components/dropdowns/#menu-alignment? I still see the broken menu...

@rohit2sharma95
Copy link
Collaborator

I still see the broken menu...

Because of the positioning of the dropdown initially.

@mdo mdo force-pushed the main-mc-popperfix branch from d0de138 to c4b6c48 Compare February 3, 2021 22:00
@XhmikosR
Copy link
Member

XhmikosR commented Feb 4, 2021

@rohit2sharma95 the issue mdo described above still happens. Was this supposed to be fixed by #32524?

@rohit2sharma95
Copy link
Collaborator

rohit2sharma95 commented Feb 4, 2021

#32524 was just to remove the margin from the dropdown. The wrong placement of dropdown-menu-end is because of the positioning (right style) IMO

@mdo
Copy link
Member

mdo commented Feb 5, 2021

Opened #32986 as an alternative for now.

@mdo
Copy link
Member

mdo commented Feb 5, 2021

Closing for #32986.

@mdo mdo closed this Feb 5, 2021
@mdo mdo deleted the main-mc-popperfix branch February 5, 2021 23: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.

Dropdown menu not positioned correctly with .dropstart
6 participants