Bug - CodeEditor - CodeEditorControl needs clean up #7884
Description
<CodeEditor>actions are built using<Tooltip>and<Button>and we have this fancy pants<CodeEditorControl>component, so we have a lot of unnecessary duplication in the code editor, and in this example toolbar. Ideally we would use<CodeEditorControl>for all the things.<CodeEditorControl>has a lot of props that are redundantly redefined or different (that might not need to be?) from<Tooltip>'s defaults. For example- entryDelay: 300 is tooltip's default
- exitDelay: 0 is different (tooltip's is 300) - does it need to be different?
- position: top is tooltip's default
- and so on...
- As a side note, ideally we would just leave all of these redundant props unset if they aren't passed, so
<CodeEditorControl>stays in sync with<Tooltip>
<CodeEditorControl>hardcodes the tooltip's trigger prop. That should be configurable with a trigger prop on<CodeEditorControl>.- You should probably be able to pass tooltipProps or something to
<CodeEditorControl>that gets passed along to<Tooltip>. - Does
<CodeEditorControl>need to set maxWidth: 100px? That's pretty narrow. Could that just use tooltip's default? - It's probably covered with the context above, but the example's copy code button needs a tooltip on focus at least.
- Why don't the exitDelay and "reset tooltip text" times sync up? They're different - they're 1600 exitDelay, and text resets at 2500. If you set them to the same, the text resets before the tooltip disappears. I found this in a few spots:
- Example toolbar exitDelay and reset text
- Code editor's exitDelay and reset text
- Copy clipboard's exitDelay and reset text (I think that's it anyways, and it's 2000 instead of 2500)
- We use execCommand('copy') in all of these places, like https://github.com/patternfly/patternfly-org/blob/main/packages/documentation-framework/helpers/copy.js
- That's officially deprecated - https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand
- That method calls for adding a form element to the DOM, and calling .select() on it to copy the text. .select() steals the browser focus, so clicking on the copy button doesn't manage focus the same way other buttons do. IMO, unless the stuff you're copying is 1) in a form element already, and 2) visibly selected, focus should be maintained on the element that triggered the copy command. It's most visible when you click the copy button at the end of any of our examples, then hit tab - that uses copy.js linked above, which appends the form element to
<body>, so hitting tab takes you to the "skip to content" button. - navigator.clipboard() seems widely supported for regular text anyways. Could we use that instead?
- I made this codepen to test - https://codepen.io/mcoker/pen/MWVdOPE
Metadata
Metadata
Assignees
Labels
No labels
Type
Projects
Status