Skip to content

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

Merged
cchaos merged 6 commits intoelastic:masterfrom
cchaos:fix/skip_link_safari
Jun 26, 2020
Merged

[EuiSkipLink] Fix TS types and Safari click issue#3665
cchaos merged 6 commits intoelastic:masterfrom
cchaos:fix/skip_link_safari

Conversation

@cchaos
Copy link
Copy Markdown
Contributor

@cchaos cchaos commented Jun 26, 2020 •
edited
Loading

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
thompsongl
thompsongl suggested changes Jun 26, 2020
View reviewed changes
Copy link
Copy Markdown
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.

@cchaos cchaos requested a review from thompsongl June 26, 2020 16:19
@myasonik
Copy link
Copy Markdown
Contributor

myasonik commented Jun 26, 2020

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
Copy Markdown

kibanamachine commented Jun 26, 2020

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

@cchaos
Copy link
Copy Markdown
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
Copy Markdown
Contributor

thompsongl commented Jun 26, 2020

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
Copy Markdown
Contributor

myasonik commented Jun 26, 2020

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
Copy Markdown
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.

cchaos
cchaos commented Jun 26, 2020
View reviewed changes
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 26, 2020

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
@thompsongl thompsongl mentioned this pull request Jul 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@thompsongl thompsongl thompsongl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cchaos @myasonik @kibanamachine @thompsongl