Tooltip for the autocalculated partition size limits

[lslezak]

Problem

• There is no hint for the users which settings can affect the auto calculated limit sizes
• https://trello.com/c/3L9aF34g/3346-2-agama-explanation-for-auto-calculated-volumes
• https://github.com/openSUSE/agama/blob/master/doc/storage_ui.md

Solution

• Add a tooltip with the items which affect the calculation

TODO

• Create a separate component for handling tooltips
• Add documentation comments
• Add unit tests

Screenshots

Icon inside the label, pointer cursor, Popover for the whole label

Notes

The Tooltip component can be configured to trigger on click instead of mouse over. Then it behaves exactly the same as the Popover component with the basic layout. So mainly the difference is in the appearance (dark vs. light background).

Old Screenshots

Previous versions

Tooltip

This uses the Tooltip component.

Popover

This uses the Popover component.

The Popover displays a closing symbol, by default it has focus which looks a bit ugly:

With disabled closing symbol:

[coveralls]

Changes unknown
when pulling 0eb7f21 on calculated_tooltip
into ** on master**.

[ancorgs]

If it's not affected then there is no tooltip because there is no "auto-calculated" label. Isn't it?

[lslezak]

Yes, that's it.

[dgdavid]

BTW, sorry for not writing it in the Trello card (I did in the chat during the planning instead, my fault) but I wonder if a Popover would be a better component for this use case. Not sure, but maybe worth it exploring the idea. That said, both looks good to me for this use case, the Tooltip and the Popover.

[dgdavid]

That said, both looks good to me for this use case, the Tooltip and the Popover

Thinking twice, I would add an "i", "?"or similar icon to activate whatever component we use. Why? To prevent users from associating that labels might have more information hidden, and "force" them to hover over all the labels for checking if there are any more things to see.

[dgdavid]

IMHO, I would prefer to have the "i" icon more integrated with the label (e.g., placing it inside, similar to the examples here https://www.patternfly.org/v4/components/label#examples).

I somehow agree, but at this moment

• Not sure how difficult the integration would be at the markup level. We're using our Em component and it will be redefined a bit. Something out of the scope of this PR, IMO.
• I see a bit confusing having two icons in the label: one clickable, the other not. So, I would remove the "icon representation" in such case, which at the end save space and probably reduce the risk of being using the wrong icon.

We could "sacrifice" the custom icon (e.g, auto icon, snapshots icon) in favor of the "i" icon when the label needs to show info.

+1

We could "sacrifice" the custom icon (e.g, auto icon, snapshots icon) in favor of the "i" icon when the label needs to show info.

No, please, do not make a label looks like a link

Use another kind of element instead of labels.

I would do that

[joseivanlopez]

IMHO, I would prefer to have the "i" icon more integrated with the label (e.g., placing it inside, similar to the examples here https://www.patternfly.org/v4/components/label#examples). I am not sure whether having the "i" outside is a good idea when there are more than one labels (e.g., auto and snapshots). I expect the "auto" label would provide the explanation about auto-calculating, and what conditions affects the calc. And the "snapshots" label would explain that snapshots is active and the effects it has, for example.

Some ideas that come to my mind:

• We could "sacrifice" the custom icon (e.g, auto icon, snapshots icon) in favor of the "i" icon when the label needs to show info.
• Add some kind of indicator (e.g., underlining the label text) to indicate that the label is clickable.
• Use another kind of element instead of labels.

[joseivanlopez]

Some ideas that come to my mind:

• We could "sacrifice" the custom icon (e.g, auto icon, snapshots icon) in favor of the "i" icon when the label needs to show info.

But, in this case, I would avoid custom icons for labels without info. I mean, I would only use the "i" icon when there is info to show, or no icon at all. IMHO, having icons for all labels make a bit difficult to easily see what icon provide info and what does not.

• Add some kind of indicator (e.g., underlining the label text) to indicate that the label is clickable.
• Use another kind of element instead of labels.

[joseivanlopez]

Some ideas that come to my mind:

• We could "sacrifice" the custom icon (e.g, auto icon, snapshots icon) in favor of the "i" icon when the label needs to show info.

But, in this case, I would avoid custom icons for labels without info. I mean, I would only use the "i" icon when there is info to show, or no icon at all. IMHO, having icons for all labels make a bit difficult to easily see what icon provide info and what does not.

On the other hand, I fear that labels in tables should always show some info, and having the "i" for all them would be a bit ... repetitive. I guess we need to find a better element for that labels in tables.

• Add some kind of indicator (e.g., underlining the label text) to indicate that the label is clickable.
• Use another kind of element instead of labels.

[dgdavid]

My thoughts related to the PR description:

The Popover displays a closing symbol, by default it has focus which looks a bit ugly:

I've been working in how the focused/active Element is displayed by the browser. It's a bit tricky and I already did some changes at #544

There is pending work related with :foucs and :focus-visible CSS pseudo selectors.

Anyway, we always can improve the uglyness :) :)

The Tooltip component can be configured to trigger on click instead of mouse over. Then it behaves exactly the same as the Popover component with the basic layout. So mainly the difference is in the appearance (dark vs. light background).

If we're going to use always the basic layout we can go ahead with the tooltip. The background is not a problem, we probably can change it by redefining a PF CSS Custom Property. Don't worry because of that.

Maybe the way could be to the same we did with the Em component: to make our own component, although initially it will be just a wrapper of a PF component. Thus, if we change our mind and want to switch from Tooltip to a Popover we can do it easily, apart from other stuff. But I fully understand that it is out of the scope of your current PBI :)

[dgdavid]

Some ideas that come to my mind:

• We could "sacrifice" the custom icon (e.g, auto icon, snapshots icon) in favor of the "i" icon when the label needs to show info.

But, in this case, I would avoid custom icons for labels without info. I mean, I would only use the "i" icon when there is info to show, or no icon at all. IMHO, having icons for all labels make a bit difficult to easily see what icon provide info and what does not.

Fully agree. That's what I meant too.

[dgdavid]

On the other hand, I fear that labels in tables should always show some info, and having the "i" for all them would be a bit ... repetitive. I guess we need to find a better element for that labels in tables.

I do not agree. Or, in other words, I'm not able to foresee that behaviour. In any case, for me it's not the same an icon that an icon inside a label 🤷‍♂️

[dgdavid]

I do not agree. Or, in other words, I'm not able to foresee that behaviour. In any case, for me it's not the same an icon that an icon inside a label man_shrugging

I meant, I do not feel that

labels in tables should always show some info

If that's the case, I fear we're misusing labels. A label should be auto explanatory. This storage case is a very special case in having to explain why something happened. Or maybe a fault in our side trying to compact a lot of information in just one word.

Not sure. I fully trust in the fact that we're going to improve and polish the UI in each iteration, but assuming now that labels show always be linked to a tooltip or a "show more" behavior is something I can't do at this time :)

[joseivanlopez]

Not sure. I fully trust in the fact that we're going to improve and polish the UI in each iteration, but assuming now that labels show always be linked to a tooltip or a "show more" behavior is something I can't do at this time :)

I don't know, but now we have two possible labels for storage ("auto" and "snapshots"). For both I expect the "i" icon. Anyway, we could start with that approach (i.e., using "i" icon or nothing) and reevalute later if needed.

[dgdavid]

Not sure. I fully trust in the fact that we're going to improve and polish the UI in each iteration, but assuming now that labels show always be linked to a tooltip or a "show more" behavior is something I can't do at this time :)

I don't know, but now we have two possible labels for storage ("auto" and "snapshots"). For both I expect the "i" icon. Anyway, we could start with that approach (i.e., using "i" icon or nothing) and reevalute later if needed.

But still being the very same special case in my eyes. Although I don't know now why you expect more explanations about snapshots in a Tooltip/Popover/i form, it just a storage section. Not enough for thinking that all labels on all tables will have an associated tooltip IMHO.

Anyway, we could start with that approach (i.e., using "i" icon or nothing) and reevalute later if needed.

That's what I think it's the best we can do right now... and even later. Start simple, improve it later if needed.

[joseivanlopez]

But still being the very same special case in my eyes. Although I don't know now why you expect more explanations about snapshots in a Tooltip/Popover/i form, it just a storage section. Not enough for thinking that all labels on all tables will have an associated tooltip IMHO.

"Snapshots" or "Encrypted" have implications. I expect to provide some explanations to the user. And somehow, I feel that labels in tables would be used to indicate very specefic active options, but as said, let's see later how it evolves.

[dgdavid]

Thanks for a very great job, @lslezak 👍 I know you have put a lot of time and love in this PBI ❤️ So, despite I have left some nitpicking comments, I think the PR deserves a review from another team member since maybe I'm a bit biased due to the fact that the approach we ended up following here does not look fully right to me (and, to be clear, I can be completely wrong).

[dgdavid]

NP: We also have an If component for this kind of scenarios where we are just displaying a component or another based on a condition.

[lslezak]

I tried that it does not look nice with multiple nested components spread over several lines:

  return (
    <If
      condition={description}
      then={
        <Popover showClose={false} bodyContent={description} {...otherProps}>
          <span className="cursor-pointer">{children}</span>
        </Popover>
      } else={
        // none or empty description, just return the children
        children
      }
    />
  );

IMHO this looks a bit better:

  if (description) {
    return (
      <Popover showClose={false} bodyContent={description} {...otherProps}>
        <span className="cursor-pointer">{children}</span>
      </Popover>
    );
  }

  // none or empty description, just return the children
  return children;

[dgdavid]

I'm a bit lost here. I think this component is redundant with the Attribute component, which returns a plain label when no description is given. And it's the only place where Description is used.

[lslezak]

The reason is that I'd like to share the code which uses Popover for displaying details or help so it can be used elsewhere. When we later decide to use Tooltip instead of Popover (or change the look) then we need to update just one place.

[joseivanlopez]

Umm, but we can do the same having only the Attribute component, right?. I mean, we can modify the internal implementation of Attribute to use a tooltip or to provide other look. And, I would use Attribute (or Tip) only for the cases in which we want a label with more info. Otherwise, I would use the Label directly.

[lslezak]

The point is that you can use a tooltip also for something else, not only Label.

[joseivanlopez]

Ah, now I got it. Thanks!

[joseivanlopez]

LGTM