SwatchTooltip component #956

supernova-at · 2019-02-25T21:29:00Z

Description

This PR adds a new SwatchTooltip component and uses it in the ProductOptions views (product page and edit mini cart item).

It updates the closed #712.

Related Issue

Closes #700 .

Motivation and Context

There was a UAT ticket for providing these tooltips.

How Has This Been Tested?

yarn test and manually on the product pages (PDPs) and in the mini-cart edit item dialog.

Screenshots (if appropriate):

Proposed Labels for Change Type/Package

FEATURE

venia-concept

Checklist:

I have read the CONTRIBUTING document.
I have linked an issue to this PR.
I have indicated the change type and relevant package(s).
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
All CI checks are green (linting, build/deploy, etc).
At least one core contributor has approved this PR.

coveralls · 2019-02-25T21:53:37Z

Coverage decreased (-0.1%) to 76.533% when pulling 1518dc2 on supernova/700 into 1272fe7 on develop.

soumya-ashok

Had a look at the implementation. The left and right padding around the label needs to be reduced, and the vertical padding increased. On second thought, off-center tip makes the tooltip look unbalanced.

Would centering the tip cause any issues with display when the tooltip needs to appear on the swatch at the left edge of the screen?

zetlen

Confirmed that this looks great in:

Chrome
Safari
Firefox
Edge
iOS Simulator
Android Emulator

One little prop type issue and then this is good to merge.

...ages/venia-concept/src/components/ProductOptions/__tests__/__snapshots__/option.spec.js.snap

supernova-at · 2019-03-13T20:40:43Z

Had a look at the implementation. The left and right padding around the label needs to be reduced, and the vertical padding increased. On second thought, off-center tip makes the tooltip look unbalanced.

Totally agree.

Would centering the tip cause any issues with display when the tooltip needs to appear on the swatch at the left edge of the screen?

It would, yeah. Would you mind if we merged this now and I create a follow up ticket to address these concerns?

soumya-ashok · 2019-03-14T14:36:55Z

@supernova-at Sure, as long as we fix the issues I'm fine with changes being addressed in a follow-up ticket. I changed the status to "approved" for my review.

supernova-at · 2019-03-14T15:11:59Z

As long as we fix the issues I'm fine with changes being addressed in a follow-up ticket.

@soumya-ashok Awesome - created #1024 for follow up.

sirugh

This looks good, but I think we have an opportunity for an a11y win here. Below are some basic guidelines for tooltips from WAI-ARIA.

The tooltip never receives focus. Focus stays on the textbox.
The tooltip widget can shown via keyboard focus or by the onmouse event.
The tooltip widget can be hidden by removing focus from the text box or by moving the mouse off the textbox.
The tooltip widget can be hidden by pressing the Escape key
The tooltip is only hidden via JavaScript and CSS selectors. If JavaScript is not available the tooltip is shown.
The element that serves as the tooltip container has role tooltip.
The element that triggers the tooltip references the tooltip element with aria-describedby.

supernova-at · 2019-03-14T20:24:18Z

we have an opportunity for an a11y win here

Any chance I could convince you to allow this in the followup ticket? 😄

sirugh · 2019-03-14T21:16:09Z

Any chance I could convince you to allow this in the followup ticket?

I don't think it's that much work to add. role="tooltip" and aria-describedby="<id>" are easy enough. The focus things are also easy I think, probably just removing z-index. Heck I've got downtime so I could look into it tomorrow if you want. Just let me know :D

jimbo · 2019-03-14T23:44:40Z

Had a look at the implementation. The left and right padding around the label needs to be reduced, and the vertical padding increased. On second thought, off-center tip makes the tooltip look unbalanced.

Agreed on all points.

supernova-at · 2019-03-18T17:25:56Z

In progress....

The tooltip never receives focus. Focus stays on the textbox.
The tooltip widget can shown via keyboard focus or by the onmouse event.
This one's a little weird at first because you could have focus on one element and be mousing over another, causing two tooltips to show simultaneously.
The tooltip widget can be hidden by removing focus from the text box or by moving the mouse off the textbox.
The tooltip widget can be hidden by pressing the Escape key
~~The tooltip is only hidden via JavaScript and CSS selectors. If JavaScript is not available the tooltip is shown.~~
Venia depends entirely on JavaScript being available so this one is invalid for us.
The element that serves as the tooltip container has role tooltip.
The element that triggers the tooltip references the tooltip element with aria-describedby.

vercel · 2019-03-20T16:29:15Z

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

sirugh · 2019-03-20T18:27:45Z

packages/venia-concept/src/components/SwatchTooltip/swatchTooltip.js

+    };
+
+    onKeyDown = event => {
+        if (event.key === 'Escape') {


Suggested change

if (event.key === 'Escape') {

if (event.key === 'Escape' || event.key === 'Tab') {

This would also hide it if you are moused in and then press tab. This could present an issue wherein there are focusable children within the button, imagine some text within the swatch, and pressing Tab should move focus to the text and keep the tooltip above the swatch.

This suggestion would help solve the problem of multiple tooltips showing at the same time. This occurs when one Swatch has focus and another is being moused over.

But it may not be worth the effort.

We'd have to add logic to SwatchTooltip to workaround the new issue that it would present, which would be duplicating a lot of what blur is supposed to tell us.

Since this isn't a form, users usually aren't navigating with keyboard and mouse.

We could add some sort of TooltipController component that ensures that only one tooltip is showing at a time, but that seems like overkill too.

I agree, it's not worth the effort. The intentions are good here—accessibility is important—but reimplementing keyboard navigation is generally a sign that we're using the wrong elements in the first place.

Rather than a list of <button> elements, product options (including swatches) should really be radio buttons, in part because this really is a form. The native mechanisms browsers have for navigating forms are actually really comprehensive, and we should be using them.

Generally speaking, if the expected behavior is that only one tooltip can be open at a time, then it's not local state: it's shared state. So it sounds like this situation does call for a "controller". Context is good for this purpose.

At my last company we had a FocusManager component that handled this sort of thing. Probably a bit overkill for now.

@jimbo great points all around.

if the expected behavior is that only one tooltip can be open at a time

This is the piece I'm iffy about. I mean, it sounds right but it is also a pretty rare occurrence so I lean towards "out of scope" here. When this thing makes it into Peregrine we can definitely generalize and make improvements to it.

What do you think?

When this thing makes it into Peregrine we can definitely generalize and make improvements to it.

Yeah, that's probably the right approach.

…in the SwatchTooltip component. - Also updates affected snapshots

…o into supernova/700-swatch_tooltips

supernova-at · 2019-03-20T18:56:14Z

PR Updated:

Removes erroneous className composition during cloneElement

jimbo · 2019-03-20T22:05:20Z

packages/venia-concept/src/components/SwatchTooltip/swatchTooltip.js

+    };
+
+    onKeyDown = event => {
+        if (event.key === 'Escape') {


I agree, it's not worth the effort. The intentions are good here—accessibility is important—but reimplementing keyboard navigation is generally a sign that we're using the wrong elements in the first place.

Rather than a list of <button> elements, product options (including swatches) should really be radio buttons, in part because this really is a form. The native mechanisms browsers have for navigating forms are actually really comprehensive, and we should be using them.

jimbo · 2019-03-20T23:29:09Z

packages/venia-concept/src/components/SwatchTooltip/swatchTooltip.js

+                onMouseLeave: over([
+                    child.props.onMouseLeave,
+                    this.onMouseLeave
+                ])


I've never seen this pattern before. Ordinarily, you would call the props callback from the class handler. Any reason we shouldn't do that here?

Also, by convention, class handlers are named handleEvent rather than onEvent.

class Foo extends Component { handleClick = event => { const { id, onClick } = this.props // this component's business event.preventDefault() // its parent's business // note: event object is treated as private // callback gets only what it needs if (typeof onClick === "function") { onClick(id) } } render() { const { handleClick } = this return <button onClick={handleClick} /> } }

Yeah that's a good call - the reason this particular case is different is because this SwatchTooltip component is the parent, not the child.

In other words:

<SwatchTooltip> <Swatch /> </SwatchTooltip>

So for a given event handler - let's say onClick - SwatchTooltip (the parent) wants its handler executed while also executing the Swatch (child)'s handler.

I suppose I could do something like

onMouseOver = () => { // parent business. this.setState({ isShowing: true }); // child business. const child = this.getChild(); // some logic that uses React.Children to get the right one if (child.props.onMouseOver) { child.props.onMouseOver(); } };

and repeat that pattern for each event handler.

I'm mostly just trying to avoid tightly coupling these components.

I'm mostly just trying to avoid tightly coupling these components.

Why's that? It seems to me that the tooltip should be a child of the Swatch, rendered by Swatch, and pretty coupled to it.

Why's that? It seems to me that the tooltip should be a child of the Swatch, rendered by Swatch, and pretty coupled to it.

Good point, that would definitely make things much simpler here. In the future we may want a generic Tooltip component like this, but that's out of scope here.

jimbo · 2019-03-20T23:33:51Z

packages/venia-concept/src/components/SwatchTooltip/swatchTooltip.js

+    };
+
+    onKeyDown = event => {
+        if (event.key === 'Escape') {


Generally speaking, if the expected behavior is that only one tooltip can be open at a time, then it's not local state: it's shared state. So it sounds like this situation does call for a "controller". Context is good for this purpose.

jimbo · 2019-03-20T23:38:03Z

packages/venia-concept/src/components/SwatchTooltip/swatchTooltip.css

+}
+
+/* The tooltip triangle that points down. */
+.tooltip:before {


Love me some pseudo-element triangles.

sirugh · 2019-03-25T20:58:30Z

We're going to merge this with the caveat that we try to figure out how to invert the parent-child relationship of the ToolTip component.

…ore Peregrine refactor

AlexeyKaryka and others added 2 commits February 25, 2019 15:22

Adds new SwatchTooltip component and uses it in ProductOptions

eeab847

Updates unit test snapshots

a155af7

supernova-at mentioned this pull request Feb 25, 2019

Create tooltip component, add to color swatch at product page #712

Closed

4 tasks

awilcoxa assigned zetlen Feb 27, 2019

soumya-ashok suggested changes Feb 28, 2019

View reviewed changes

supernova-at changed the base branch from release/2.0 to develop March 6, 2019 17:05

Merge branch 'develop' into supernova/700

f650c76

vercel bot deployed to staging March 6, 2019 17:05 View deployment

zetlen suggested changes Mar 11, 2019

View reviewed changes

...ages/venia-concept/src/components/ProductOptions/__tests__/__snapshots__/option.spec.js.snap Outdated Show resolved Hide resolved

Merge branch 'develop' into supernova/700

e32dc68

vercel bot deployed to staging March 11, 2019 18:45 View deployment

Merge branch 'develop' into supernova/700

2c06482

vercel bot deployed to staging March 12, 2019 17:00 View deployment

Merge branch 'develop' into supernova/700

8dea1f5

vercel bot deployed to staging March 13, 2019 20:18 View deployment

Removes itemIndex attribute from button rendered by Swatch component

edbbbef

vercel bot deployed to staging March 13, 2019 20:48 View deployment

soumya-ashok previously approved these changes Mar 14, 2019

View reviewed changes

sirugh requested changes Mar 14, 2019

View reviewed changes

Merge branch 'develop' into supernova/700

1001406

vercel bot deployed to staging March 18, 2019 16:36 View deployment

Adds ARIA best practices to SwatchTooltip

2e425af

sirugh reviewed Mar 20, 2019

View reviewed changes

supernova-at added 2 commits March 20, 2019 13:48

Reverts a change that added an erroneous className to a cloned child …

3fa99ff

…in the SwatchTooltip component. - Also updates affected snapshots

Merge branch 'supernova/700' of github.com:magento-research/pwa-studi…

1c718c2

…o into supernova/700-swatch_tooltips

vercel bot deployed to staging March 20, 2019 18:50 View deployment

sirugh previously approved these changes Mar 20, 2019

View reviewed changes

supernova-at assigned sirugh Mar 20, 2019

jimbo suggested changes Mar 20, 2019

View reviewed changes

sirugh removed their assignment Mar 21, 2019

Merge branch 'develop' into supernova/700

215c1df

vercel bot deployed to staging March 21, 2019 19:50 View deployment

sirugh self-assigned this Mar 25, 2019

sirugh unassigned zetlen Mar 25, 2019

Merge branch 'develop' into supernova/700

e97eead

sirugh dismissed their stale review via e97eead March 26, 2019 14:28

vercel bot temporarily deployed to staging March 26, 2019 14:28 Inactive

Merge branch 'develop' into supernova/700

fb43b83

vercel bot deployed to staging March 26, 2019 14:30 View deployment

Add comment re: future of Tooltip component

93571ca

vercel bot deployed to staging March 26, 2019 15:24 View deployment

Move tooltip to ProductOptions to discourage reuse in the interim bef…

1518dc2

…ore Peregrine refactor

vercel bot deployed to staging March 26, 2019 15:39 View deployment

sirugh merged commit 48dabc1 into develop Mar 26, 2019

sirugh deleted the supernova/700 branch March 26, 2019 16:00

sirugh removed their assignment Mar 26, 2019

sirugh mentioned this pull request Mar 27, 2019

feature: Layered Navigation (Filters) #797

Merged

8 tasks

cherdman added this to the Next PWA Release (Core 2.3.2) milestone Apr 29, 2019

sirugh added the version: Minor This changeset includes functionality added in a backwards compatible manner. label May 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SwatchTooltip component #956

SwatchTooltip component #956

supernova-at commented Feb 25, 2019 •

edited

Loading

coveralls commented Feb 25, 2019 •

edited

Loading

soumya-ashok left a comment

zetlen left a comment

supernova-at commented Mar 13, 2019

soumya-ashok commented Mar 14, 2019 •

edited

Loading

supernova-at commented Mar 14, 2019

sirugh left a comment

supernova-at commented Mar 14, 2019

sirugh commented Mar 14, 2019

jimbo commented Mar 14, 2019

supernova-at commented Mar 18, 2019 •

edited

Loading

vercel bot commented Mar 20, 2019

sirugh Mar 20, 2019

supernova-at Mar 20, 2019

jimbo Mar 20, 2019

jimbo Mar 20, 2019

sirugh Mar 21, 2019

supernova-at Mar 21, 2019

jimbo Mar 21, 2019

supernova-at commented Mar 20, 2019

jimbo Mar 20, 2019

jimbo Mar 20, 2019

supernova-at Mar 21, 2019 •

edited

Loading

jimbo Mar 21, 2019

supernova-at Mar 21, 2019

jimbo Mar 20, 2019

jimbo Mar 20, 2019

sirugh commented Mar 25, 2019

	if (event.key === 'Escape') {
	if (event.key === 'Escape' \|\| event.key === 'Tab') {

SwatchTooltip component #956

SwatchTooltip component #956

Conversation

supernova-at commented Feb 25, 2019 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Proposed Labels for Change Type/Package

Checklist:

coveralls commented Feb 25, 2019 • edited Loading

soumya-ashok left a comment

Choose a reason for hiding this comment

zetlen left a comment

Choose a reason for hiding this comment

supernova-at commented Mar 13, 2019

soumya-ashok commented Mar 14, 2019 • edited Loading

supernova-at commented Mar 14, 2019

sirugh left a comment

Choose a reason for hiding this comment

supernova-at commented Mar 14, 2019

sirugh commented Mar 14, 2019

jimbo commented Mar 14, 2019

supernova-at commented Mar 18, 2019 • edited Loading

vercel bot commented Mar 20, 2019

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

supernova-at commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernova-at Mar 21, 2019 • 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

sirugh commented Mar 25, 2019

supernova-at commented Feb 25, 2019 •

edited

Loading

coveralls commented Feb 25, 2019 •

edited

Loading

soumya-ashok commented Mar 14, 2019 •

edited

Loading

supernova-at commented Mar 18, 2019 •

edited

Loading

supernova-at Mar 21, 2019 •

edited

Loading