Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiSkipLink] Fix TS types and Safari click issue #3665

Merged
merged 6 commits into from
Jun 26, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jun 26, 2020

A couple issues arose when trying to use this component in Kibana.

Props

Interactive props

There was a simple code error where the component's props was not all the props. It was referencing just the EuiSkipLinkProps instead of the Prop one which includes the PropsForAnchor and PropsForButton props to allow for onClick and such. So TS threw an error when trying to add onClick

destinationId

This prop was required but is too specific to only one possible use case/consuming ability. For instance, Discover's "Skip to bottom of table" button still has an Angular wrapper and so it needed to do the functionality via an onClick and not an href. I just made this one optional.

Update: Is still required

Safari

The browser had a weird state when you couldn't click the button with a mouse when in the focus state. As soon as you mouse down, the button would disappear again. I was able to fix this by appending :not(:active) to the selector for when to hide the button. This seems to fix the issue.

Before
Screen Recording 2020-06-26 at 11 52 AM

After
Screen Recording 2020-06-26 at 11 53 AM

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • [ ] Checked in mobile
  • Checked in Safari and Firefox
  • Props have proper autodocs
  • [ ] Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@cchaos cchaos requested a review from thompsongl June 26, 2020 15:55
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just a couple type export things.

src/components/accessibility/skip_link.tsx Outdated Show resolved Hide resolved
src/components/accessibility/skip_link.tsx Outdated Show resolved Hide resolved
src/components/accessibility/skip_link.tsx Outdated Show resolved Hide resolved
@cchaos cchaos requested a review from thompsongl June 26, 2020 16:19
@myasonik
Copy link
Contributor

button still has an Angular wrapper and so it needed to do the functionality via an onClick and not an href

Why is that? The href should be able to take you to any ID on the page so I don't see how JS libs affect that unless Angular is messing up our IDs. The limited scope of destinationId was intentionally created to limit abuse/misuse of skip links so I'm hesitant to open it up to the infinite possibilities of onClick...

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3665/

@cchaos
Copy link
Contributor Author

cchaos commented Jun 26, 2020

The limited scope of destinationId was intentionally created to limit abuse/misuse of skip links

Yeah I understand this, but we need to be flexible at the same time. We enforce good/appropriate behavior through our documentation and examples while still being open about "necessary" vs "best practice". We shouldn't shut consumers out from using certain components when it's the appropriate component.

@thompsongl
Copy link
Contributor

The limited scope of destinationId was intentionally created to limit abuse/misuse of skip links so I'm hesitant to open it up to the infinite possibilities of onClick

onClick has always been available as valid prop for the component (so long as the required destinationId prop was also provided). If onClick isn't supposed to be an option, the types here could be simplified quite a bit, I think.

@myasonik
Copy link
Contributor

Intended use of skip links is to:

  • be a link
  • have an href that points to an ID on the same page
  • that ID be valid and unique

In the original PR, I argued for it to be as prescriptive and restrictive as possible to that. If y'all want to continue down that path, allowing onClick in the first place would be a bug. But, I think that might not be inline with EUI, so if y'all want to open it up, this PR seems good (docs are good, snippet is happy path, etc).

@cchaos
Copy link
Contributor Author

cchaos commented Jun 26, 2020

I would like to keep onClick available as an option. However, I will re-require destinationId. At the very least, we're expecting an id but consumers using other ways/libraries to hook up to linking or have issues with hash urls (like EUI docs do), they have the ability to provide onClick to create the interaction.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3665/

@cchaos cchaos merged commit 88f1167 into elastic:master Jun 26, 2020
@cchaos cchaos deleted the fix/skip_link_safari branch June 26, 2020 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants