refactor tooltip and popover components #747

mcoker · 2018-09-20T23:21:51Z

fixes #688

refactored arrow positioning in both components
update minWidth/minHeight global vars for target size
updates to a11y and docs for both components
refactor handlebars code

Not going to address the demo/functionality with this PR. We will demo that when the React components are built and we can use them in a demo.

patternfly-build · 2018-09-20T23:48:59Z

Deploy preview for pf-next ready!

Built with commit c56a6b3

https://deploy-preview-747--pf-next.netlify.com

matthewcarleton · 2018-09-21T14:59:47Z

src/patternfly/components/Popover/popover-arrow.hbs

@@ -0,0 +1,6 @@
+<div class="pf-c-popover__arrow{{#if popover-body--modifier}} {{popover-body--modifier}}{{/if}}"


these should be arrow not body for the modifier and attribute.

doh! fixed.

matthewcarleton · 2018-09-21T15:00:33Z

src/patternfly/components/Popover/popover-arrow.hbs

+  {{#if popover-body--attribute}}
+    {{{popover-body--attribute}}} 
+  {{/if}}>
+  {{> @partial-block}}


Do we need to include {{> @partial-block}} here? I'm not sure. I was wondering this when I was looking at how we want to set our rules. It's not hurting anything, so I don't know if it matters. What do you think?

I don't see the harm in adding the standard --modifier, --attribute and @partial-block hbs code to all partials. I didn't have it initially, but it is more consistent to add it to any partial, and even though it doesn't do anything at the moment, it's there if we want to use it in the future. And doesn't add any bloat to PF itself, just a few lines of code to the workspace.

matthewcarleton · 2018-09-21T16:18:59Z

src/patternfly/components/Tooltip/docs/code.md

-| `.pf-m-right` | `.pf-c-tooltip` | Puts arrow to the right of the tooltip's content. |
-| `.pf-m-top` | `.pf-c-tooltip` | Puts arrow on top of the tooltip's content. |
-| `.pf-m-bottom` | `.pf-c-tooltip` | Puts arrow on the bottom of the tooltip's content. |
+| `.pf-c-tooltip` | `<div>` |  Creates a tooltip. Always use with a modifier class. |


Can you add required to this.

matthewcarleton · 2018-09-21T16:23:51Z

Really nice job! Just a few things I've noticed.

matthewcarleton · 2018-09-21T18:17:21Z

I'm looking at the design and seeing that the tooltip is white. I wonder if we need to pull in design here and talk about the dark tooltips. Should there be a modifier for the dark/light tooltip/popover?

jgiardino · 2018-09-25T20:28:30Z

src/patternfly/components/Tooltip/docs/code.md


 ## Accessibility

 | Attribute | Applied To | Outcome |
 | -- | -- | -- |
-| `aria-describedby="[id value of applicable content]"` | `.pf-c-tooltip` | Gives the tooltip an accessible description by referring to the tooltip content that describes the primary message or purpose of the dialog. Not used if there is no static text that describes the popover. |
+| `role="tooltip"` | `.pf-c-tooltip` | Adds a tooltip role to the tooltip component. **Required**|
+| `aria-describedby="[id of .pf-c-tooltip__content]"` | `*` | Gives the element that triggers the tooltip an accessible description by referring to the `id` of the `.pf-c-tooltip__content` element in the tooltip, describing the primary message or purpose of the element. Not used if there is no static text that describes the element. |


Using aria-describedby matches the pattern described in the wai-aria example for tooltip, but there are two use cases that are described in the Inclusive Components article for tooltips:

One where the tooltip provides a description using aria-describedby, which matches this example

One where the tooltip provides a label using aria-labelledby.

I'd like for us to be able to support both use cases. The second one will be a common use case wherever an icon is included without accompanying text.

What are your thoughts on updating this section to something like this?

Attribute Applied To Outcome

aria-describedby="[id of .pf-c-tooltip__content]" or aria-labelledby="[id of .pf-c-tooltip__content]" [element that triggers the tooltip] Makes the contents of the tooltip accessible to assistive technologies by associating the tooltip text with the element that triggers the tooltip. Use aria-labelledby if the tooltip provides a label for the element. Use aria-describedby if the element already has an accessible label and it's different from the tooltip text.

Oh, I forgot to include "Required" in my suggestion above. Either aria-labelledby or aria-describedby is required when using a tooltip to make the text accessible to screen readers.

Yes looks great! Will update.

jgiardino · 2018-09-25T21:37:48Z

src/patternfly/components/Popover/docs/code.md

-| `aria-describedby="[id value of applicable content]"` | `.pf-c-popover` | Gives the popover an accessible description by referring to the popover content that describes the primary message or purpose of the dialog. Not used if there is no static text that describes the popover. |
+| `aria-labelledby="[id of .pf-c-popover__header-title]"` | `.pf-c-button` | Gives the element that triggers the popover an accessible name by referring to popover's title. **Required when .pf-c-popover__header-title is present** |
+| `aria-label="[title of popover]"` | `.pf-c-button` | Gives the element that triggers the popover an accessible name. **Required when .pf-c-popover__header-title is _not_ present** |
+| `aria-describedby="[id of .pf-c-popover__body]"` | `.pf-c-button` | Gives the element that triggers popover an accessible description by referring to the popover's body text, describing the primary message or purpose of the dialog. Not used if there is no static text that describes the popover. |


I actually think that the previous descriptions for aria-label, aria-labelledby, and aria-describedby were correct. I was thinking that a popover is very similar to a modal, and so a lot of the aria attributes we are using for the modal would apply to the popover, too. And the previous version of these attributes were a direct copy from the modal example.

Please me know if you found other examples that suggest a different method for handling popovers.

Ah yes, I believe you're right. I was considering tooltips and popovers to be similar in terms of a11y, but a popover is basically a modal. And the changes I made are how Bootstrap handles their popovers (with the aria attributes on the button that triggers it). I'll revert this section of the file.

jgiardino · 2018-09-25T21:45:57Z

src/patternfly/components/Popover/docs/code.md

-| `aria-describedby="[id value of applicable content]"` | `.pf-c-popover` | Gives the popover an accessible description by referring to the popover content that describes the primary message or purpose of the dialog. Not used if there is no static text that describes the popover. |
+| `aria-labelledby="[id of .pf-c-popover__header-title]"` | `.pf-c-button` | Gives the element that triggers the popover an accessible name by referring to popover's title. **Required when .pf-c-popover__header-title is present** |
+| `aria-label="[title of popover]"` | `.pf-c-button` | Gives the element that triggers the popover an accessible name. **Required when .pf-c-popover__header-title is _not_ present** |
+| `aria-describedby="[id of .pf-c-popover__body]"` | `.pf-c-button` | Gives the element that triggers popover an accessible description by referring to the popover's body text, describing the primary message or purpose of the dialog. Not used if there is no static text that describes the popover. |
 | `aria-popover="true"` | `.pf-c-popover` | Tells assistive technologies that the windows underneath the current popover are not available for interaction. **Required**|


As I mentioned in my previous comment, this section was a copy/paste from the modal, and I think this was a find/replace mistake. There is no aria-popover attribute, but there is an aria-modal attribute.

@mcarrano Would you consider a popover to be a modal element? Is your expectation that popovers automatically disappear if the user clicks on elements outside the popover?

jgiardino · 2018-09-25T21:51:56Z

src/patternfly/components/Popover/docs/code.md

-| `.pf-c-popover__header` | `.pf-c-popover__content` |  The header text area of the popover. |
-| `.pf-c-popover__title` | `.pf-c-popover__header` |  The actual popover title. |
-| `.pf-c-popover__body` | `.pf-c-popover__content` |  The popover's body text. ** Required.** |
+| `.pf-c-popover` | `<div>` |  Creates a popover. Always use it with a modifier class. **Required** |


Do you think consumers will understand that "Always use it with a modifier class" means to use a class like .pf-m-left? I wonder if just updating the description for the modifier classes to something like "A modifier class that puts the arrow to the left of the popover's content" would help connect those dots.

I'll add that, I don't think it will hurt and will add more context. Just so I'm sure what you're suggesting, instead of saying "Always use it with a modifier class", you're suggesting something like "Always use it with a modifier class that positions the popover relative to the element that triggered it."

Glad you mentioned this because currently these modifier descriptions are wrong (for example, pf-m-left positions the popover to the left of the element that triggered it, so the arrow is actually on the right), and I don't think we should describe what the modifier does by where the arrow is, and instead describe it as a class that positions the popover. I made that change in the tooltip component and forgot to make it here.

Oh and I meant to mention that the "always use with a modifier class" is something existing that we use on title, alert, button, and badge, too. Do you think it needs to be updated in those components as well?

I was thinking that we would call out the classes that were modifier classes. So anytime we use a modifier class, we start the description by saying "A modifier class that...".

Class Applied To Outcome

.pf-m-left .pf-c-popover A modifier class that positions the popover to the left of the popover trigger

But now that you described what you were thinking, I think either could work.

As for making this change in other component examples, I like having consistency. Do you want to update this example and see what everyone thinks?

Ah, I see. Personally I think it's fine to not start modifier classes with "A modifier class that...", which assumes that folks understand that .pf-m- denotes a class that modifies something in the component. I think it might be repetitive to have each modifier class start with that, too. Personally I think "always use with a modifier class" is explanatory enough, too - it means always use one of the .pf-m- classes in the component... but I could be making assumptions, and having the additional descriptions could be helpful :) Agreed about consistency.

jgiardino · 2018-09-25T22:02:44Z

@michael-coker Your updates are great! I made a few comments about the documentation. Please let me know if you have any questions about them.

mcoker · 2018-09-25T22:37:47Z

@jgiardino thank you so much for the review!! appreciate you setting me straight on the a11y stuff :) Updated the PR with those changes if you want to have another look.

jgiardino · 2018-09-26T15:03:36Z

👍 Looks good to me. I like your updates to the Usage section better than my suggestion. I agree that my suggestion would get redundant. Being someone who's usually clueless, I'm inclined to over-document. 😆

mcoker · 2018-09-26T19:47:10Z

@jgiardino thanks!! would you mind marking the PR as reviewed/approved?

jgiardino

LGTM!

andresgalante · 2018-09-28T13:12:39Z

@michael-coker great Job, just one thing:

The popover doesn't work well on dark environments. I don't know what the design ask for, do they want it to change to dark bg and white fonts, like the card, or they want it to stay white bg with dark fotns.

We need to define this with @mceledonia or @kybaker and code it.

mcoker · 2018-09-28T13:46:28Z

@andresgalante sorry, I should have included this in the issue. Matt mentioned this, too, so I asked @kylebaker and here was his response.

@mcoker Follow the design suggested in issue 474 - the dark. 14px font and 16 px margin

matthewcarleton · 2018-09-28T18:34:09Z

src/patternfly/components/Popover/examples/popover-bottom-example.hbs

+{{#> popover popover--modifier="pf-m-bottom"}}
+  {{#> popover-content}}
+    {{#> popover-header}}
+      {{#> popover-header-title  popover-header-title--attribute='id="popover-bottom-header"'}}


delete space after title

matthewcarleton

LGTM! Just one little nit pick on whitspace :)

… to match AAA a11y

…escription of popover modifier classes, add aria-labelledby to tooltip a11y docs, be more descriptive with "always use with modifier class" in tooltip and popover docs, move role and aria-modal attrs from popover-content to the popover component

LHinson · 2018-09-28T19:12:17Z

@michael-coker great Job, just one thing:

The popover doesn't work well on dark environments. I don't know what the design ask for, do they want it to change to dark bg and white fonts, like the card, or they want it to stay white bg with dark fotns.

We need to define this with @mceledonia or @kybaker and code it.

Filed #772 and added to the next alpha to revisit this issue separate from this PR.

patternfly-build · 2018-10-01T18:12:44Z

🎉 This PR is included in version 1.0.49 🎉

The release is available on:

Your semantic-release bot 📦🚀

mcoker requested review from matthewcarleton, jameslau, srambach and jgiardino September 20, 2018 23:21

matthewcarleton reviewed Sep 21, 2018

View reviewed changes

jgiardino reviewed Sep 25, 2018

View reviewed changes

mcoker force-pushed the issue-688 branch from c9a2c4c to fbe51b0 Compare September 25, 2018 22:30

jgiardino approved these changes Sep 27, 2018

View reviewed changes

matthewcarleton reviewed Sep 28, 2018

View reviewed changes

matthewcarleton approved these changes Sep 28, 2018

View reviewed changes

mcoker added 6 commits September 28, 2018 13:51

refactor(popover): refactor popover handlebars and CSS

2412cdf

popover docs typo

62bfe35

refactor(tooltip): refactor tooltip handlebars and CSS

1cbd12d

docs and a11y updates

6091792

refactor(vars): update minWidth and minHeight target area var to 44px…

d59b010

… to match AAA a11y

rework popover arrow positioning

20ebf1d

mcoker added 7 commits September 28, 2018 13:51

add ids, update docs

71db7c1

refactor transform vars

d2e1185

typo in popover-arrow.hbs

c70dde7

update accessibility guide target size

4e74c83

add **required** where necessary, remove unnecessary popover css

e67b20d

whitespace

c56a6b3

mcoker force-pushed the issue-688 branch from fbe51b0 to c56a6b3 Compare September 28, 2018 18:51

matthewcarleton merged commit b7d0a1b into patternfly:master Sep 28, 2018

LHinson mentioned this pull request Sep 28, 2018

Addressing Popover Color #772

Closed

mcoker mentioned this pull request Oct 8, 2018

Remove workspace classes from code samples in Popover/Tooltip #526

Closed

mcoker deleted the issue-688 branch December 16, 2019 23:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor tooltip and popover components #747

refactor tooltip and popover components #747

mcoker commented Sep 20, 2018

patternfly-build commented Sep 20, 2018 •

edited

Loading

matthewcarleton Sep 21, 2018

mcoker Sep 21, 2018

matthewcarleton Sep 21, 2018

mcoker Sep 21, 2018

matthewcarleton Sep 21, 2018

matthewcarleton commented Sep 21, 2018

matthewcarleton commented Sep 21, 2018

jgiardino Sep 25, 2018 •

edited

Loading

jgiardino Sep 25, 2018

mcoker Sep 25, 2018

jgiardino Sep 25, 2018

mcoker Sep 25, 2018 •

edited

Loading

jgiardino Sep 25, 2018

jgiardino Sep 25, 2018

mcoker Sep 25, 2018

mcoker Sep 25, 2018

jgiardino Sep 25, 2018

mcoker Sep 25, 2018

jgiardino commented Sep 25, 2018

mcoker commented Sep 25, 2018

jgiardino commented Sep 26, 2018

mcoker commented Sep 26, 2018

jgiardino left a comment

andresgalante commented Sep 28, 2018

mcoker commented Sep 28, 2018

matthewcarleton Sep 28, 2018

mcoker Sep 28, 2018

matthewcarleton left a comment

LHinson commented Sep 28, 2018 •

edited

Loading

patternfly-build commented Oct 1, 2018

		@@ -0,0 +1,6 @@
		<div class="pf-c-popover__arrow{{#if popover-body--modifier}} {{popover-body--modifier}}{{/if}}"

refactor tooltip and popover components #747

refactor tooltip and popover components #747

Conversation

mcoker commented Sep 20, 2018

patternfly-build commented Sep 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewcarleton commented Sep 21, 2018

matthewcarleton commented Sep 21, 2018

jgiardino Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcoker Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgiardino commented Sep 25, 2018

mcoker commented Sep 25, 2018

jgiardino commented Sep 26, 2018

mcoker commented Sep 26, 2018

jgiardino left a comment

Choose a reason for hiding this comment

andresgalante commented Sep 28, 2018

mcoker commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewcarleton left a comment

Choose a reason for hiding this comment

LHinson commented Sep 28, 2018 • edited Loading

patternfly-build commented Oct 1, 2018

patternfly-build commented Sep 20, 2018 •

edited

Loading

jgiardino Sep 25, 2018 •

edited

Loading

mcoker Sep 25, 2018 •

edited

Loading

LHinson commented Sep 28, 2018 •

edited

Loading