Page MenuHomePhabricator

(MS 6) Create popover component
Closed, ResolvedPublic13 Estimated Story Points

Description

As a DS designer and developer, I want to have a new popover component available, because it is needed in the Simple Query Builder.

ACs

  • The popover component is implemented according to the Figma specs and follows the behavior and accessibility principles defined in the component documentation.
  • The popover's component-level tokens (available in this PR) are created according to the naming convention used to style the component and provide all the needed styling values
  • The popover is documented in Storybook according to the specifications (see below)

Position

Popovers do not have states, but we need to make it possible for implementers to define their position in relation to the element that triggers them (see notes):

  • Top: right, center, left
  • Bottom: right, center, left
  • Left: bottom, center, top
  • Right: bottom, center, top

Storybook:

This component has its own pages in Storybook, included by alphabetical order (see structure):

  • A Popover page including controls: they allow users to enter custom text to replace the default content and to change the popover position

behavioral ACs

  • can be closed/opened programmatically
    • has prop: isShown or isVisible
    • emits event update:isShown or update:isVisible so that it can be used with the .sync modifier
  • is closed on click outside
  • is closed on hover-out

technical ACs

  • note in Changelog
  • is exported in main.ts

Notes:

  • It would be ideal if the position of the popover could be defined via prop
  • Regarding the popover triggering event: Ideally, the component should be revealed on click as a baseline, but implementers should be allowed to expand it (e.g. via prop) so mouse users can also see the popover on hover if needed.
  • The closing event should also be controlled by the implenenters, and ideally follow the component documentation recommendations (unless we want to enforce this behavior in the component – or components): popovers that open on hover, should close when hover is removed from the trigger. Otherwise, only clicking outside the popover should dismiss it.
  • A very similar (but slightly more complex) version of this component was implemented by the Tainted References team (see component in Storybook)

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
amy_rc renamed this task from Create tooltip component to (MS 6) Create tooltip component.Jan 27 2021, 2:48 PM
amy_rc moved this task from Incoming to Backlog on the Wikidata Query Builder board.
Sarai-WMDE renamed this task from (MS 6) Create tooltip component to (MS 6) Create popover component.Jan 28 2021, 11:44 AM
Sarai-WMDE updated the task description. (Show Details)
Sarai-WMDE updated the task description. (Show Details)

@Michael brought up this important question about the popover:

"Do we have a plan/idea how we want to get it where it is designed to appear?"

There are two approaches that @ItamarWMDE and I find convenient. In both cases, the link between the popover and its trigger is defined by the popover component:

  1. Using a wrapper. This is, adding (via a slot or a title prop) a parent element that wraps the popover together with its trigger. Both Vuetify and Material design applied a version of this approach. While it might not be very correct semantically, the method does seem convenient. Please note that only Vuetify's approach would allow us to insert elements other than text in the popover (e.g. links), and for that reason, it might be the most advantageous.
  1. Using a for attribute to connect the popper to its trigger's id. This would look something like this:

<popover for="foo" left center> Popover content </popover>
<button id="foo">Button label</button>

<popover for="foo" left center> Popover content </popover>
<button id="foo">Button label</button>

Exactly, as an additional comment, in my mind it would be great if we could pass any CSS selector in the for property to apply the popover to, but I understand if we need to rely solely on id or name.

So the "for attribute"-scenarion would then roughly look like

<template>
    <Lookup ... tooltipFor="valueTooltip" />
    <Popover forId="valueTooltip">...</Popover>
</template>

?

I still have to wrap my head around how the "Using a wrapper" scenario would change the props of e.g. the Lookup component.

Though really, we can simplify that "for attribute"-scenario to

<template>
    <Lookup ... :hasTooltip="true" @tooltip:clicked="showValueTooltip = !showValueTooltip" />
    <Popover show.sync="showValueTooltip">Content of Value Tooltip</Popover>
</template>

No fancy attributes and CSS selectors needed. just an icon-button that is shown if some prop is true and an extra event that is emitted when that button is clicked.

The "wrapper scenario" could essentially be achieved in one of two ways:

  1. Naive Wrapper (MaterialUI)
<Popover title="popover text" top left>
    <Lookup ... />
</Popover>
  1. Parenting Wrapper (Vuetify)
<Popover top left>
     <template v-slot:activator>
        <Lookup .../>
     </template>
     Whatever content we want in this popover including <a>links</a> and other <Components />
</Popover>

Yes, but the <Lookup .../> has to somehow know that it is wrapped by a Popover so that it can render the . So we have to somehow adjust its props.

I don't think it's the Lookup's responsibility to render the Icon, if you want an icon to trigger the popup then an Icon component should be the target of it. Otherwise, we're making a design decision for potentially other implementations of this popover.

I agree in principle.
But, in practical terms, we somehow have to get that icon robustly next to the label. I don't really worry much about the popover itself. But placing that Icon from the outside next to the Label while treating the entire Lookup component as a black box, seems ambitious.

Perhaps it's time to consider those prefix and suffix slots we've been talking about before. Otherwise, we might want to leave the label open as a slot to be able to include non-textual content:

<Lookup ...>
  <template v-slot:label>
     <span>I'm your label <Icon type="info" id="note-123" /></span>
     <Popover for="note-123" top left>I'm something..</Popover>
  </template>
</Lookup>

I don't think it's the Lookup's responsibility to render the Icon, if you want an icon to trigger the popup then an Icon component should be the target of it. Otherwise, we're making a design decision for potentially other implementations of this popover.

I think the taintedrefs team (perhaps with less frontend experience so take this all with a pinch of salt) came to the conclusion that the "Popper" shouldn't know much about its surroundings. Both including what it was popping up near but also what might be triggering it to pop up/down.

The only real logic we put it in was knowing how to close itself (e.g click the 'x' or focusing off it). The rest of the appearance/disappearance was handled higher up the application. It looks like vuetify offers controlling the tool-tip in a similar way to what tainted-refs did but using v-model rather than rather than having a v-if element wrapping it (which I think might be nicer). You could even consider this being the only api and not include a way to inject a triggering component at all.

A question it might be worth asking is "will popover only ever be triggered by an element being clicked or hovered or will they sometimes be triggered by some other application state change?"

It looks like vuetify offers controlling the tool-tip in a similar way to what tainted-refs did but using v-model rather than rather than having a v-if element wrapping it (which I think might be nicer). You could even consider this being the only api and not include a way to inject a triggering component at all.

I think it's a good idea in a system that creates components specifically made for a purpose such as tainted refs, it's also a great MVP. But beyond that, I think the strength of the popover lies in the fact that you don't need to specify absolute positions and that you can always anchor it to any other element in your page.

Otherwise, we can just instruct the QB team to use a div with a v-if and some clever CSS tricks to ensure the position is close to what we want visually. Even in the example I think you are referring to, you still need an "activator" node to position the popover: https://vuetifyjs.com/en/components/tooltips/#visibility

Either or, yes, this kind of functionality (programmatically opening a popover) should be part of the API of this component, but we should probably introduce it only when we truly need it.

Perhaps it's time to consider those prefix and suffix slots we've been talking about before. Otherwise, we might want to leave the label open as a slot to be able to include non-textual content:

<Lookup ...>
  <template v-slot:label>
     <span>I'm your label <Icon type="info" id="note-123" /></span>
     <Popover for="note-123" top left>I'm something..</Popover>
  </template>
</Lookup>

Yeah, making the label into a slot is also the thing my mind came up with yesterday while it was supposed to fall asleep 😅 . Probably, that's the way to go :)

I think it's a good idea in a system that creates components specifically made for a purpose such as tainted refs, it's also a great MVP. But beyond that, I think the strength of the popover lies in the fact that you don't need to specify absolute positions and that you can always anchor it to any other element in your page.

Gotcha! It's certainly a snazzy feature. I think at the time we had some idea that the "Popper" might see use outside tainted-refs (hence why there is a really rather generic Popper and then an implementation) but at the time I don't think I had any ideas about it being any more than a dumb component (i.e. one who's parent controls both it's position and visibility).

Otherwise, we can just instruct the QB team to use a div with a v-if and some clever CSS tricks to ensure the position is close to what we want visually. Even in the example I think you are referring to, you still need an "activator" node to position the popover: https://vuetifyjs.com/en/components/tooltips/#visibility

Maybe there is also the "attach" thing? And of course they allow manual positioning by props (but as you said above that's not really very useful).

Either or, yes, this kind of functionality (programmatically opening a popover) should be part of the API of this component, but we should probably introduce it only when we truly need it.

👍 no point in building things we don't need.

I was just thinking that designing all popovers to *have* to have a trigger to which they are attached is an explicit decision. I could see this being as case of

making a design decision for potentially other implementations of this popover

In any case, I'm not that sure that my chipping in with random thoughts from >1 year ago is very helpful 😀. Just feel free to give me a nudge if there might be anything from tainted-refs I can drag out of my memory that would actually be helpful for this rather than just possible ramblings orthogonal to the direction of travel.

amy_rc claimed this task.