Skip to content
This repository has been archived by the owner on Jun 11, 2021. It is now read-only.

feat(react): place dropdown with Popper #25

Merged
merged 4 commits into from
Feb 14, 2020

Conversation

francoischalifour
Copy link
Owner

@francoischalifour francoischalifour commented Feb 12, 2020

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) and end (bottom right)

interface AutocompleteProps {
  // ...
  dropdownPlacement: 'start' | 'end';
}
<Autocomplete
  // ...
  // Usage for DocSearch
  dropdownPlacement="end"
  // Usage for InstantSearch (by default)
  dropdownPlacement="start"
/>

For now, users can only choose the placement of the dropdown (start or end). 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

image

end

image

Customize

Users are able to tweak the margin in CSS because this Popper configuration doesn't reset the margin style property.

.algolia-autocomplete-dropdown {
  margin-top: 0;
}

@media (min-width: 600px) {
  .algolia-autocomplete-dropdown {
    margin-top: 5px;
  }
}
See outdated

EDIT: this was fixed in e6cfd05.

To add custom responsive styles, user will have to use !important in CSS because Popper add styles to the dropdown DOM elements (which takes precedence over class).

.algolia-autocomplete-dropdown {
  margin-top: -5px !important;
}

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.

Copy link
Collaborator

@s-pace s-pace left a 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

@FezVrasta
Copy link

FezVrasta commented Feb 12, 2020

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.

@francoischalifour
Copy link
Owner Author

@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 margin: 0px style which breaks the implementation in this case. How would you go about adding styles in CSS classes that don't get overridden by Popper? This is the style value that it generates:

{
  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 */;
}

@FezVrasta
Copy link

FezVrasta commented Feb 13, 2020

margins on popper elements are not supported, consumers should define popper offsets with the offset modifier. https://popper.js.org/docs/v2/modifiers/offset/

I'm not 100% sure why do we apply margin but it may be to prevent issues when users accidentally set the margin.
https://popper.js.org/docs/v2/migration-guide/#4-remove-all-css-margins

@francoischalifour
Copy link
Owner Author

francoischalifour commented Feb 13, 2020

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):

image

On mobile however, we'd like to allow such designs (full width, full height, no space between the search box and the dropdown):

image

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?

@FezVrasta
Copy link

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.

@FezVrasta
Copy link

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:

{
  name: 'unsetMargins',
  enabled: true,
  fn: ({ state }) => { state.styles.popper.margin = null; }
  requires: ['computeStyles'],
  phase: 'beforeWrite',
}

@FezVrasta
Copy link

@atomiks suggested to do something like this:

const yourLibrary = userProvidedMediaQuery => createPopper(x, y, {
  modifiers: [
    {
      name: 'offset',
      options: {
        offset: () => {
          if (userProvidedMediaQuery.matches) {
            return [0, 0];
          } else {
            return [0, 10];
          }
        }
      }
    } 
  ]
});

yourLibrary(window.matchMedia('(max-width: 500px)'));

This should allow Popper to conditionally apply different offsets based on the browser window width.

@francoischalifour
Copy link
Owner Author

@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 margin in Popper cause trouble?

@francoischalifour
Copy link
Owner Author

Note that there seem to have a mistyping in the library because this raises a TypeScript error and undefined doesn't unset the CSS property:

state.styles.popper.margin = null;

Type 'null' is not assignable to type 'string | undefined'.

@atomiks
Copy link

atomiks commented Feb 14, 2020

I think it should be an empty string "" instead of null.

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

@francoischalifour
Copy link
Owner Author

That sounds good – I'm satisfied with the current solution!

Thank you @FezVrasta and @atomiks, your help and reactivity are very much appreciated!

@atomiks
Copy link

atomiks commented Feb 14, 2020

I do think there is a dev warning if using margins though (2.0.1+)...

@francoischalifour
Copy link
Owner Author

I don't get a warning with this Popper configuration with version 2.0.6.

Copy link
Collaborator

@eunjae-lee eunjae-lee left a 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!

@francoischalifour francoischalifour merged commit ca38070 into next Feb 14, 2020
@francoischalifour francoischalifour deleted the feat/react-renderer-popper branch February 14, 2020 10:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants