[Pagination] Disable buttons trigger onPrevious/onNext when using the keys and render the tooltips

[tleunen]

Issue summary

3 things I noticed about the pagination, keys and disable state:

1. The tooltip is shown on disabled pagination buttons
2. When the tooltip appears on a disabled button, it's hard to hide it. Hovering out a normal button hide the tooltip, but it's not the case for the disabled button.
3. Triggering the key event triggers the previous/next callback, even if the button is disabled

Expected behavior

• I'm not sure if 1. is expected or not. Having it or not could be valid I guess.
• Hovering out of the button should hide the tooltip though.
• Key presses should not trigger the callbacks if hasPrevious/hasNext is false (button disabled).

Actual behavior

See issue.

Steps to reproduce the problem

1. Use the codesandbox and play each bullet points in the summary

Reduced test case

https://codesandbox.io/s/ywrxx3qp8j

👋 Thanks for opening your first issue. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

[tleunen]

Looks like 3. will be fixed by #643

[AndrewMusgrave]

Thanks for the issue @tleunen # 3 will be fixed in our next release. @sarahill do you have any thoughts on 1?

[sarahill]

It feels like a bug to have the tooltip show up if the button is disabled. I would lean toward not displaying the tooltip for disabled buttons since there is no action you can take.

[ruairiphackett]

The CSS solution here fixes # 2 if applied to the disabled Pagination Button:

.Button {
  &:disabled {
    pointer-events: none;
  }
}

I'm not sure if there's any scenario where we might need pointer-events on a disabled button though? Or if it is better to not display the tooltip at all for disabled buttons.

Forked sandbox with the fix for # 2: https://codesandbox.io/s/xoyqy6y5mw

[ruairiphackett]

I can put in a PR with the above if it is an acceptable fix.

[yourpalsonja]

@dpersing @AndrewMusgrave any thoughts on the above fix?

[dpersing]

I'm not sure if 1. is expected or not. Having it or not could be valid I guess.
Hovering out of the button should hide the tooltip though.
Key presses should not trigger the callbacks if hasPrevious/hasNext is false (button disabled).

Agreed that the tooltip should disappear when hovering out. Since we're using tooltips to instruct users of the keyboard shortcuts, I like the idea of still showing the tooltip on hover, but this creates an inequitable experience for people who aren't using the mouse, so I'm inclined to say we should suppress tooltips on disabled buttons.

That however brings up a question about whether or not using those tooltips to instruct users about the keyboard shortcuts is the best way to provide that information. (There are also more general concerns about single-key shortcuts.) Not sure who we'd talk to about this, but it might be good to consider folding the pagination shortcuts in to any larger keyboard shortcuts provided across the platform.

[dleroux]

@dpersing are you saying we should still show the tooltip on disabled buttons?