patternfly-react icon indicating copy to clipboard operation
patternfly-react copied to clipboard

fix(CodeEditor): use codeEditorControls and clean up overall

Open nicolethoen opened this issue 3 years ago • 5 comments

What: Closes #7884

  • we now use <CodeEditorControl> for all dynamically built controls in CodeEditor.tsx.
  • we now pass tooltipProps to <CodeEditorControl> that gets passed along to <Tooltip>.
  • Code editor now uses tooltip's default prop values unless consumer overrides them with tooltipProps
  • Deprecated the previous prop that were passed to the tooltip (they still work)
  • Copy to clipboard button in the code editor now triggers with keyboard focus
  • exitDelay and "reset tooltip text" times sync up
  • Updated copy functions to use navigator.clipboard()

nicolethoen avatar Sep 02 '22 18:09 nicolethoen

Preview: https://patternfly-react-pr-7931.surge.sh

A11y report: https://patternfly-react-pr-7931-a11y.surge.sh

patternfly-build avatar Sep 02 '22 19:09 patternfly-build

Clipboard copy exit behavior is a bit different after you click to copy.

  • When I click to copy, switch tabs, then come back to the tab with the tooltip, the tooltip shows.

    • Actually this is happening everywhere. Seems to be because the tooltip trigger has focus.

I do believe that's happening because the timeout doesn't get triggered to start until you move focus off the tooltip, or mouseout of the tooltip. I think that's intended?

  • If you click "copy" (the tooltip then displays "copied"), then wait a second before moving your mouse away, the text will change to "copy" again before hiding. Or for a really fun time, click it and keep moving your mouse on and off the button and the tooltip never goes away! I'm not sure if that's preferred over the current behavior? Or if that could be seen as a bug or a bug fix?

    • Code editor does this, too, but is doing that on main now currently, too.

Given that the 'Copied' text is on its own timer, and it does not get reset when you do things like move your mouse away from the trigger or move focus off the trigger, it's inevitable that the more time you spend restarting the tooltip hide timer by moving your mouse over the trigger, or by returning focus to the trigger, that the 'copied' text will timeout before the tooltip itself.

This looks like a pre-existing thing, but the code block copy button's tooltip seems to always say "successfully copied to clipboard" after you click it - it never resets. Should it?

hm... that might be out of scope. But I can see how easy that'd be to update. Maybe I can roll it in.

nicolethoen avatar Sep 19 '22 17:09 nicolethoen

Clipboard copy exit behavior is a bit different after you click to copy.

  • When I click to copy, switch tabs, then come back to the tab with the tooltip, the tooltip shows.

    • Actually this is happening everywhere. Seems to be because the tooltip trigger has focus.

I do believe that's happening because the timeout doesn't get triggered to start until you move focus off the tooltip, or mouseout of the tooltip. I think that's intended?

  • If you click "copy" (the tooltip then displays "copied"), then wait a second before moving your mouse away, the text will change to "copy" again before hiding. Or for a really fun time, click it and keep moving your mouse on and off the button and the tooltip never goes away! I'm not sure if that's preferred over the current behavior? Or if that could be seen as a bug or a bug fix?

    • Code editor does this, too, but is doing that on main now currently, too.

Given that the 'Copied' text is on its own timer, and it does not get reset when you do things like move your mouse away from the trigger or move focus off the trigger, it's inevitable that the more time you spend restarting the tooltip hide timer by moving your mouse over the trigger, or by returning focus to the trigger, that the 'copied' text will timeout before the tooltip itself.

This looks like a pre-existing thing, but the code block copy button's tooltip seems to always say "successfully copied to clipboard" after you click it - it never resets. Should it?

hm... that might be out of scope. And the setting of the 'copied' text is completely consumer controlled. So it's completely configurable.

nicolethoen avatar Sep 19 '22 18:09 nicolethoen

I do believe that's happening because the timeout doesn't get triggered to start until you move focus off the tooltip, or mouseout of the tooltip. I think that's intended?

Yeah it makes sense why it's happening, it's just a bit odd? In case I didn't describe it well, here's a recording. I'm just clicking to copy, waiting for the tooltip to disappear, then going to another tab and coming back - the tooltip fires again when I switch back.

Sep-19-2022 15-21-21

mcoker avatar Sep 19 '22 20:09 mcoker

Given that the 'Copied' text is on its own timer, and it does not get reset when you do things like move your mouse away from the trigger or move focus off the trigger, it's inevitable that the more time you spend restarting the tooltip hide timer by moving your mouse over the trigger, or by returning focus to the trigger, that the 'copied' text will timeout before the tooltip itself.

Works for me and makes sense, it's just different than the current behavior. Seems like the current timeout for the "copied" text closes the tooltip without considering any hover behavior that would reset the tooltip timer as it does now.

mcoker avatar Sep 19 '22 20:09 mcoker

I'll dig a little deeper

nicolethoen avatar Sep 30 '22 13:09 nicolethoen

@tlabaj I added the console warnings, but also will address this org issue I created as soon as this merges to suppress those console warnings coming from the documentation-framework. It was a first attempt at clean up work from a month ago in the documentation-framework that prompted this clean up in the first place.

nicolethoen avatar Oct 13 '22 20:10 nicolethoen