Select: Portal select menu to document.body

[ashharrison90]

What this PR does / why we need it:

• portals the Select menu to document.body
• auto positions the Select menu for better placement
• closes the Select menu when scrolling anywhere outside of the Select menu
• adds a new portal z-index value set to higher than the modal
• rewrites Portal component as a functional component so it can utilise useTheme2
• rewrites SelectBase tests as RTL tests
• updates other unit tests
  • react-select-event has an extra option to specify where the menu is contained. e.g.

    await selectEvent.select(selectEl, 'Option 2');

    becomes

    await selectEvent.select(selectEl, 'Option 2', { container: document.body });

    or use the new util method exposed from @grafana/ui:

    import { selectOptionInTest } from '@grafana/ui';
    await selectOptionInTest(selectEl, 'Option 2');

• updates e2e tests
  • as the menu is no longer within the Select container, need to look for options on the body. e.g.

    e2e.components.TimeZonePicker.container()
      .should('be.visible')
      .within(() => {
        e2e.components.Select.input().should('be.visible').click();
        e2e.components.Select.option().should('be.visible').contains(toTimeZone).click();
      });

    should now be written as

    e2e.components.TimeZonePicker.container()
      .should('be.visible')
      .within(() => {
        e2e.components.Select.input().should('be.visible').click();
      });
    e2e.components.Select.option().should('be.visible').contains(toTimeZone).click();

• have also updated the e2e util method selectOption here: https://github.com/grafana/grafana/blob/main/packages/grafana-e2e/src/flows/selectOption.ts to work with the new structure.
• since the menu is portalled to body, wrapping the Select in something like ClickOutsideWrapper will no longer work. the same functionality can be achieved using Select's onCloseMenu/isOpen props.

some screenshots:

Which issue(s) this PR fixes:

Special notes for your reviewer:

[torkelo]

Awesome job! this is tricky stuff, and we have had max-width / overflow hidden/auto before in the Modal but it issues (esp with selects) made us change that back. There are some other Modals with selects in them (like dashboard SaveAs modal).

[Clarity-89]

Looks good! We don't have any use cases of Selects in Modal in Enterprise as far as I can see. Regarding the value mappings and modal in general, I think a nice improvement would be if the button actions wouldn't be a pat of scrolling content and always stayed visible during overflow, but this is probably out of scope of this PR.

[ashharrison90]

@Clarity-89 +1 on that. i think every major design library out there (bootstrap, material, carbon, etc.) separates the actions from the content for this reason.

[torkelo]

An idea would to add a footer prop to Modal that automatically wraps the footer content in a ModalButtonRow, and the footer would live outside the content div

[tskarhed]

The more react-select stuff we expose in our API, the harder it will be to move away from it. I don't think it's in the current todo, but the Select component has been a real pain.

[tskarhed]

Maybe it is okay in this case

[ashharrison90]

hmm, that's a good point. i'm not sure how we can avoid it though.
we could attach all selects to document.body by default? that would have a pretty wide impact though, and is presumably slightly worse for performance.
ideally we'd want something where the select detects it's in a modal and attaches itself to document.body automatically 🤔

[tskarhed]

I played around a bit with the portaling etc. last year, and it never ended up good. When this is merged we might have to keep an eye out for Select bugs.

Let me just check a few places before you merge this PR.

[ashharrison90]

An idea would to add a footer prop to Modal that automatically wraps the footer content in a ModalButtonRow, and the footer would live outside the content div

yep, i don't make raising that/taking it on as an enhancement following this PR 👍

[tskarhed]

Okay, let's go! Be on the lookout for weird Select bugs though!

[ashharrison90]

@tskarhed @torkelo what do you think about using context to determine where to attach the Select menu? 🤔

we could export a context object from SelectBase - something like SelectMenuTargetContext - default it to undefined and wrap the modal like:

    <Portal>
      <div className={cx(styles.modal, className)}>
        <SelectMenuTargetContext.Provider value={document.body}>
          <div className={headerClass}>
            {typeof title === 'string' && <DefaultModalHeader {...props} title={title} />}
            {typeof title !== 'string' && title}
            <div className={styles.modalHeaderClose}>
              <IconButton surface="header" name="times" size="xl" onClick={onDismiss} />
            </div>
          </div>
          <div className={cx(styles.modalContent, contentClassName)}>{children}</div>
        </SelectMenuTargetContext.Provider>
      </div>
      <div
        className={styles.modalBackdrop}
        onClick={onClickBackdrop || (closeOnBackdropClick ? onDismiss : undefined)}
      />
    </Portal>

that way we wouldn't have to expose any new props on SelectBase or ValuePicker, and it would apply to every modal and every component wrapping Select

[ashharrison90]

ok, i've updated this to portal Select to document.body in all cases and updated the unit tests accordingly. the select menu now also closes when you scroll outside of the select menu.

the only thing left to discuss is what we do about z-index. in this PR the select menu overlays everything, including modals. i can imagine there's cases where that's not desirable.

but when a select is within a modal, it has to overlay the modal, else it's broken. so it seems like we'll need to make it configurable in some way. either by exposing a prop, or setting it via context, or something else.

[mckn]

Love that you included instructions on how to change the tests in this PR! Will help people that might run into this issue! 🙌🏻

[torkelo]

Awesome job on this ash!

only thing that can be improved would be a test helper function to select options so you not have to always pass in the container argument, should make it easier for to change this in the future without updating all tests (I hope we do no need to!!)

[ashharrison90]

unit test util method created. once everything goes green i'll merge 🤞

[dprokop]

was that it? :)

[ashharrison90]

oooo maybe, forgot about that change 😂