-
Notifications
You must be signed in to change notification settings - Fork 11
feat(react): place dropdown with Popper #25
Conversation
Co-authored-by: Federico Zivolo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can not do that without dependency, this LGTM
If you'd like to make the popper wide as its reference element, you could add a custom modifier to alter the popper size: https://codepen.io/FezVrasta/pen/gOpaoeL To override the styles applied by Popper you can do the same thing I did in the codepen above in the "sameWidth" modifier. |
@s-pace For now we're using a subset of Popper's API. If we considerer that it's too much for our use, we can remove the dependency as long as we don't leak its API. @FezVrasta Thank you for the snippet, this is going to be useful. I don't understand why Popper is opinionated about adding the {
position: absolute;
left: 0px;
top: 0px;
margin: 0px; /* this is not needed and is too opinionated for us */
right: auto;
bottom: auto;
transform: /* dynamically computed */;
} |
margins on popper elements are not supported, consumers should define popper offsets with the I'm not 100% sure why do we apply |
With DocSearch, we are used to adding a margin on desktop so that the search box and the dropdown don't touch (mainly because the two elements don't have the same width): On mobile however, we'd like to allow such designs (full width, full height, no space between the search box and the dropdown): For this kind of style features, users would expect to be able to achieve this with only CSS. However, with this solution, it wouldn't be the case. Does that make sense? |
Yes it does, unfortunately margins cause a lot of positioning issues so we had to remove them from the equation. As an alternative, you could have the popper container be a transparent element, and inside it position the dropdown element, with the needed conditional margins. |
If you decide to use CSS margins and take the risks, you can add this modifier to unset them and leave the user-defined one:
|
@atomiks suggested to do something like this:
This should allow Popper to conditionally apply different offsets based on the browser window width. |
@atomiks' solution doesn't allow users to provide their own CSS theme, which is the main point of this discussion 🙂. The goal here is not to find a one-time solution but rather to empower users to not have to think it terms of JavaScript API for such a feature. I'll go with #25 (comment) for now, thanks! Just to anticipate what problems can arise, what are the main cases where ignoring |
Note that there seem to have a mistyping in the library because this raises a TypeScript error and state.styles.popper.margin = null;
|
I think it should be an empty string The margins cause issues when the popper flips to a different axis (e.g. bottom to right), if that never happens it should be okay to use margins |
That sounds good – I'm satisfied with the current solution! Thank you @FezVrasta and @atomiks, your help and reactivity are very much appreciated! |
I do think there is a dev warning if using margins though (2.0.1+)... |
I don't get a warning with this Popper configuration with version 2.0.6. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exposed API seems good!
Description
This PR adds support for dropdown placement, based on Popper.js.
API
Although Popper allows to show floating elements on the left, right, top and bottom (each with vertical and horizontal variants), this API only supports the two main use cases for an autocomplete experience:
start
(bottom left) andend
(bottom right)For now, users can only choose the placement of the dropdown (
start
orend
). The Popper.js API is not leaked – I believe this is enough. If ever power users want to access the modifiers, we can either create an API or advise them to create their own renderer.Preview
start
end
Customize
Users are able to tweak the margin in CSS because this Popper configuration doesn't reset the
margin
style property.See outdated
EDIT: this was fixed in e6cfd05.
To add custom responsive styles, user will have to use
!important
in CSS because Popper addstyles
to the dropdown DOM elements (which takes precedence overclass
).This removes the hardcoded
offset
in JavaScript.I came across the
applyStyles
option documentation but it's incomplete. I'm not sure if there's a better way to override the computed styles (cc @FezVrasta).A pattern that must be possible is that the dropdown takes the full width and height on mobile. I'd appreciate your input on how that would be possible with Popper @FezVrasta.