fix(code-snippet): remove focus from snippet container #5202
Conversation
ghost
requested review from
|
Deploy preview for the-carbon-components ready! Built with commit 1ffae8c https://deploy-preview-5202--the-carbon-components.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit 870e3a7 |
asudoh
requested review from
a team and
laurenmrice
and removed request for
a team
|
Deploy preview for carbon-components-react failed. Built with commit 870e3a7 https://app.netlify.com/sites/carbon-components-react/deploys/5e30b827b22c38000821c2ae |
|
Deploy preview for carbon-elements ready! Built with commit 1ffae8c |
|
Deploy preview for carbon-components-react ready! Built with commit 1ffae8c https://deploy-preview-5202--carbon-components-react.netlify.com |
| .#{$prefix}--snippet--single .#{$prefix}--snippet-container, | ||
| .#{$prefix}--snippet--multi .#{$prefix}--snippet-container { | ||
| @include focus-outline('reset'); | ||
| } | ||
|
|
There was a problem hiding this comment.
this isn't needed if we are removing the tabIndex={0} from the element right?
|
@tw15egan unfortunately the single line code snippets need to receive focus since they are scrollable. Without it, a keyboard user doesn't have visual access to the whole snippet. This change definitely works for the multi-line snippets though! |
|
@abbeyhrt hmm, makes sense... Not sure what we want to do in regards to that focused state then. |
|
Looking at a few other design systems, and none of their code components receive focus 🤔. The focus goes straight to the copy button. We might just have to live with the browser focus if we want to keep that functionality |
|
is there no way around creating a focus that has to go outside to accommodate the horizontal scrollbar? i guess if anything if its only for single line in some browsers we could leave it for now. |
|
@tw15egan I do think the browser focus being misshapen looks less weird than our focus styles misshapen. It seems like this 16px padding-bottom is making the focus the way it is and it seems to be there to vertically center the text, is there a way to vertically center the text without that padding? |
|
This is just one of those components that tends to look different in various browsers/os due to the way the system handles scrolling, i.e I'm hesitant to change the padding / internal structure, which could cause more issues (List of issues) with misalignment in browsers, just to get a perfect focus state. It seems like more work than the perceived benefit. I'm good with closing this and the original focus issue with a won't fix tag. |
|
@tw15egan that's totally fair, that list of issues hurts my head haha. I'm good with closing this and keeping the original focus! 👍 |
|
Can we still keep the changes you did for the multiline code snippet since we don't need it on that variant? I am fine with keeping the single line as it was if there isn't an easy fix for that. |
|
Just removing focus? Yeah we can add that into that variant |
tw15egan
force-pushed
the
remove-snippet-focus
branch
from
9cbb647
to
2f6ce92
Compare
tw15egan
force-pushed
the
remove-snippet-focus
branch
from
8e0b9f3
to
b71eb23
Compare
|
@laurenmrice just pushed some updates, let me know if this works 👍 |
|
@emyarod updated, if you want to take a 2nd look |
|
Go for it! I'll detach the issue from this PR and just leave a ref. |
Refs #2304
Removes focus outline from the main text part of the Code Snippet (MultiLine only). The focus will now go directly to the copy button, then show more.
Changelog
New
Removed
tabindexandroleon multiline code snippetTesting / Reviewing
Tab through / click on
CodeSnippetand ensure no focus is given to the container. The focus should move to the copy button.