fix(example): increase code editor button tooltip size, other cleanup #3162
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
PF4 preview: https://patternfly-org-pr-3162-v4.surge.sh/v4 |
evwilkin
approved these changes
Aug 26, 2022
Member
evwilkin
left a comment
evwilkin
left a comment
There was a problem hiding this comment.
LGTM - thanks for all the cleanup! 💪
Collaborator
|
I opened this react issue and am taking a look |
nicolethoen
approved these changes
Aug 29, 2022
jessiehuff
pushed a commit
to jessiehuff/patternfly-org
that referenced
this pull request
Oct 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #3137
--
I noticed a bunch of other discrepancies between the buttons, so I tried updating all of them to use
<CodeEditorControl>, and with @nicolethoen's help, we discovered that passingexitDelayfrom a state var (isCopied) doesn't work if there are multipletriggervalues passed... or something like that. Got a little tricky for me, and our fearless leader @nicolethoen is going to look at it.This PR
maxWidththat widens it a bit<CodeEditorControl>and individually to<Tooltip>and<Button>to keep the examples consistentfocusas a value fortriggerand for the tooltip to move to the button - wasn't working on the<form>Some things I noticed that @nicolethoen may clean up with her work, or may need new issues for:
<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 exampleentryDelay: 300is tooltip's defaultexitDelay: 0is different (tooltip's is 300) - does it need to be different?position: topis tooltip's default<CodeEditorControl>stays in sync with<Tooltip><CodeEditorControl>hardcodes the tooltip'striggerprop. That should be configurable with atriggerprop on<CodeEditorControl>.tooltipPropsor something to<CodeEditorControl>that gets passed along to<Tooltip>.<CodeEditorControl>need to setmaxWidth: 100px? That's pretty narrow. Could that just use tooltip's default?exitDelayand "reset tooltip text" times sync up? They're different - they're 1600exitDelay, 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:exitDelayand reset textexitDelayand reset textexitDelayand reset text (I think that's it anyways, and it's 2000 instead of 2500)execCommand('copy')in all of these places, like https://github.com/patternfly/patternfly-org/blob/main/packages/documentation-framework/helpers/copy.js.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 hittab- that usescopy.jslinked above, which appends the form element to<body>, so hittingtabtakes you to the "skip to content" button.