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

Improve button link usage #5409

Closed
wants to merge 29 commits into from
Closed

Conversation

rafawendel
Copy link
Member

@rafawendel rafawendel commented Feb 25, 2021

What type of PR is this? (check all applicable)

  • Feature

Description

This PR is a split on #5365 due to its staleness.
It aims to fix issues on interactivity and improve keyboard accessibility.

All anchors <a> need to be replaced by proper semantic components other than those in viz-lib.

The following behavior for links, buttons, link-like and button-like components is aimed:

  • Links that look like links: <Link> - Renders <a>
  • Buttons that look like buttons: <Button> - Renders Antd <Button>
  • Links that look like buttons: <Link.Button> - Renders Antd <Button>, which is rendered as an <a>
  • Text Buttons "buttons that look like links": <PlainButton> - Renders styleless <button>

@rafawendel
Copy link
Member Author

Note: <Menu.Item> has accessibility on its own (via keyboard arrows), but it is overridden by the inner buttons (accessible via tabbing) due to they not being actionable. The ideal scenario is to combine both approaches, which will be tracked in the a11y epic.

@rafawendel rafawendel marked this pull request as ready for review March 8, 2021 20:30
@@ -100,6 +100,10 @@ strong {
cursor: pointer;
}

.clickable:disabled {
Copy link

Choose a reason for hiding this comment

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

Not sure there's an action to take, yet, but I have mixed feelings about this. Anything can take the classname clickable, but not anything can be disabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

What about .clickable button:disabled?

@@ -391,6 +391,18 @@
left: 0;
Copy link

Choose a reason for hiding this comment

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

I'm leaving a comment here so we can keep a discussion in the thread.

Is there any chance we can break this PR into even smaller PRs? It looks like there are quite a few styling changes, as well as component updates. I'm having trouble determining how best to QA / ensure there are no regressions.

Do you have another proposal? I don't want to stonewall you or anything, but I don want to make sure I can limit the number of iterations / time spent verifying changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's complicated. I did think about it, but the thing is that this PR resulted in a lot of visual changes, which I had to solve in order to be mergeable. What is sort of possible but somewhat weird is having the fixes for those in master before we merge it. LMK what is your opinion on that.

An alternative is to review those last commits primarily, since they are where the styling changes are amassed.

Copy link

Choose a reason for hiding this comment

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

What if we just did one button type at a time?

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.

2 participants