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

Deprecate CSS tooltip #1936

Merged
merged 4 commits into from
Feb 11, 2022
Merged

Deprecate CSS tooltip #1936

merged 4 commits into from
Feb 11, 2022

Conversation

khiga8
Copy link
Contributor

@khiga8 khiga8 commented Feb 11, 2022

Related: https://github.com/github/accessibility/issues/670

What are you trying to accomplish?

This PR deprecates tooltip. These tooltips are frequently being applied on static elements making them completely inaccessible for groups of our users.

What approach did you choose and why?

We are currently in process of upstreaming a more accessible tooltip to PVC. That work is not complete yet, but I think it's fine to move this to deprecated to discourage usage.

What should reviewers focus on?

If I set this to deprecated, what effect will it have? One painpoint I'm seeing is that these tooltips are frequently being applied to static elements. Is it worth updating examples to not set tooltips on static elements, even though if this is deprecated?

Also, should we explain why we're deprecating this?

Are additional changes needed?

  • No, this PR should be ok to ship as is. 🚢

@changeset-bot
Copy link

changeset-bot bot commented Feb 11, 2022

🦋 Changeset detected

Latest commit: 7dd10eb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@khiga8
Copy link
Contributor Author

khiga8 commented Feb 11, 2022

All the examples here set tooltipped on span elements. We strongly want to discourage this. Though I'm moving this to deprecated, would is still be worth updating the examples? I'm not sure how deprecation works.

@khiga8 khiga8 marked this pull request as ready for review February 11, 2022 01:00
@khiga8 khiga8 requested a review from a team as a code owner February 11, 2022 01:00
@khiga8 khiga8 requested a review from jonrohan February 11, 2022 01:00
@jonrohan
Copy link
Member

jonrohan commented Feb 11, 2022

All the examples here set tooltipped on span elements. We strongly want to discourage this. Though I'm moving this to deprecated, would is still be worth updating the examples? I'm not sure how deprecation works.

Yeah I think there's no harm in updating the examples.

In addition to what you've done here, you should add the classes to https://github.com/primer/css/blob/kh-deprecate_tooltip_please/src/deprecations.json with null as the replacement, ie. "tooltipped": null, this will inform the DeprecatedInPrimer erb linter in github/github and stop people from adding more classes.

@khiga8
Copy link
Contributor Author

khiga8 commented Feb 11, 2022

@jonrohan Neat linter!! I'm concerned we will need to add linter disable comment to every file given the extensive use of tooltips. I've proposed a PR in erb-lint to allow linter disable by offense (instead of the file-level disable we've hack implemented), but if that does not go through maybe it is worth converting this to a counter.

@jonrohan jonrohan merged commit c53ecdf into main Feb 11, 2022
@jonrohan jonrohan deleted the kh-deprecate_tooltip_please branch February 11, 2022 18:27
@primer-css primer-css mentioned this pull request Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants