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

Rename sr-only/sr-only-focusable to visually-hidden/visually-hidden-focusable #31139

Merged
merged 14 commits into from
Jul 3, 2020

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Jun 21, 2020

To be more representative of the fact that these are not necessarily "screen reader" specific, but actually apply to assistive technologies in general (and also things like Alexa/Siri/etc). So .v-hidden and .v-hidden-focusable.

Goes hand-in-hand with #31133

To be more representative of the fact that these are not necessarily "screen reader" specific, but actually apply to assistive technologies in general (and also things like Alexa/Siri/etc). Goes hand-in-hand with #31133
@patrickhlauke
Copy link
Member Author

This is a breaking change, so no backporting. tried to strike a balance of brevity here with .v-hidden rather than .visually-hidden.

And yes, it's possibly silly/inconsequential, but the term sr is loaded with false assumptions, and would be good to transition away from it going forward...so doing it as a breaking change in a new version seems the most opportune time

@ffoodd
Copy link
Member

ffoodd commented Jun 21, 2020

I think this is a very good idea. Two things, though:

  • I'm afraid .v-hidden could be confusing, meaning visibility for some people.
  • maybe the file itself should be renamed? _screenreader.scss does not make much sense, does it?

After renaming, it wouldn't make sense otherwise
@patrickhlauke
Copy link
Member Author

patrickhlauke commented Jun 21, 2020

@ffoodd i think even expanding this to .visually-hidden it will still be confusing/confused with visibility for some devs. Not sure there's a better way/naming around this (that is also not overly verbose). as for the second point, see #31133 (i filed that separately because even if we don't want to go through all this class renaming, the documentation should be clearer/more reflective of the reality anyway)

@patrickhlauke
Copy link
Member Author

related:

Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

:chef_kiss: Looks great!

@patrickhlauke
Copy link
Member Author

so are we happy/ok with .v-hidden (relatively short) or want to go for full-on .visually-hidden (perhaps clearer but more wordy)?

@patrickhlauke
Copy link
Member Author

patrickhlauke commented Jun 25, 2020

also, thoughts on #31133, and whether to backport that one or not?

@XhmikosR
Copy link
Member

TBH I don't know if v-hidden is better or not. I feel like it more ambiguous, but sr was also an abbreviation.

@ffoodd
Copy link
Member

ffoodd commented Jun 25, 2020

I'd be in favor of .visually-hidden.

@voltaek
Copy link
Contributor

voltaek commented Jun 25, 2020

If I saw v-hidden in some Bootstrap markup I would assume it was the utility for visibility: hidden;. I had to go reference the docs to actually check what the utility for visibility is (it's .visible and .invisible) because I rarely use it.

Using .visually-hidden seems like a good solution to that.

@patrickhlauke
Copy link
Member Author

so despite the lengthier classname, it seemingly preferred to go with .visually-hidden . .visually-hidden-focusable for clarity (and at least for the former, precedent of https://a11yproject.com/posts/how-to-hide-content/). @mdo?

@mdo
Copy link
Member

mdo commented Jun 26, 2020

I'm good with the longer name. Honestly every time I see it I think "very hidden" more than I think "visually hidden" lol.

@MartijnCuppens
Copy link
Member

Joining camp longer name

@patrickhlauke
Copy link
Member Author

right, renamed the classes/mixins to the longer but clearer variant. and on reflection, probably little use in backporting #31133 ... just concentrating on v5 for this.

in short, i think this is ready for merging, and after that #31133 would be good to merge as well. unless this needs to wait for #31122 @XhmikosR ?

@patrickhlauke
Copy link
Member Author

sorry, a bit of back and forth, but i think the migration.md is now as it should be... sanity check from @XhmikosR ?

@XhmikosR
Copy link
Member

XhmikosR commented Jul 3, 2020

Just FYI that's not a rebase, not sure how you resolved the commits but in the future please rebase and squash your patches.

@patrickhlauke
Copy link
Member Author

i followed the command line instructions given here by github. sorry i'm not a git expert...

@XhmikosR XhmikosR merged commit 10690dd into main Jul 3, 2020
@XhmikosR XhmikosR deleted the v5-sronly-rename-vhidden branch July 3, 2020 11:38
@XhmikosR XhmikosR changed the title Rename sr-only/sr-only-focusable Rename sr-only/sr-only-focusable to visually-hidden/visually-hidden-focusable Sep 29, 2020
olsza pushed a commit to olsza/bootstrap that referenced this pull request Oct 3, 2020
* Rename `sr-only`/`sr-only-focusable`

To be more representative of the fact that these are not necessarily "screen reader" specific, but actually apply to assistive technologies in general (and also things like Alexa/Siri/etc). Goes hand-in-hand with twbs#31133

Co-authored-by: XhmikosR <[email protected]>
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.

6 participants