-
-
Notifications
You must be signed in to change notification settings - Fork 57
Improve gutenberg compatibility for tooltips #748
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
base: master
Are you sure you want to change the base?
Changes from 8 commits
db46ce7
a8889b7
af570fc
f557a7e
4152aef
6c8ca31
95f5da9
c0914bd
ddea9c4
2b4c493
266b955
64ae984
c8cacba
5cb5761
9a3d561
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've used nesting in the CSS, which I didn’t see elsewhere. Here, I mostly like it for visually conveying hierarchy and it’s by no means necessary. It’s supported in all evergreen browsers since 2023.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's defer nesting until a major release. I'm in favor, but mixed coding style changes can be confusing to maintain. Also, so much extra CSS to accommodate a specific page type for the tooltip, which otherwise works universally, seems like a workaround for a bug—are you certain this isn't an upstream issue?
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tooltip colors are currently handled via I only apply patches when there's literally no feasible fix/workaround upstream.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I hadn’t used Now, as a way to keep colors as before without having to add to the inline CSS, I've pushed c8cacba, that proposes to use CSS variables. No sweat if that’s off the table for any reason—it seemed worth a try. I also put in a brief effort to understand why the GB admin accent color isn’t the same as what TSF pulls from the global. I didn’t find out but I did find that contrast issue is seemingly not considered one upstream:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
While it might be considered an upstream issue I doubt it would be considered a bug. If their Popover component’s API had a few more features this could likely be reduced. There’s another approach that could be tried but I can’t say if it’d reduce this by much. It would also require |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| :root { | ||
| /* Prevents a shift from the scrollbar gutter being added (although visually imperceptible) | ||
| that happens when the popover containing the tooltip causes overflow. The shift causes the | ||
| wrap element’s rectangle to be read at the shifted position but by the next aniation frame | ||
stokesman marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| it’s different since the popover has repositioned itself to not overflow. This seems safe | ||
| to add and although ideally Gutenberg might add this itself. */ | ||
stokesman marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| overflow-x: hidden; | ||
| } | ||
|
|
||
| .tsf-gbc-tooltip-root { | ||
| .tsf-gbc-tooltip-popover { | ||
| pointer-events: none; | ||
| filter: drop-shadow(0 0 1px rgba( 0, 0, 0, .6 ) ); | ||
|
|
||
| & > .components-popover__content { | ||
| /* Unset the Gutenberg popover’s min-content width that would otherwise require | ||
| setting width or min-width on either the tooltip or its content. */ | ||
| width: unset; | ||
| } | ||
| .tsf-tooltip-arrow { | ||
| /* Hide our arrow in Gutenberg (to use theirs instead). */ | ||
| display: none; | ||
| } | ||
| & > .components-popover__arrow { | ||
| width: 16px; | ||
| height: 16px; | ||
|
|
||
| &:before { | ||
| /* Hide Gutenberg’s arrow’s before pseudo because it’s only used to whiteout their | ||
| popover’s border where the arrow sits. */ | ||
| display: none; | ||
| } | ||
| .components-popover__triangle-bg { | ||
| fill: var(--wp-admin-theme-color); | ||
| } | ||
| .components-popover__triangle-border { | ||
| /* Hide the arrow stroke/border */ | ||
| display: none; | ||
| } | ||
| } | ||
| } | ||
| .tsf-tooltip { | ||
| /* The popover component needs this to not be absolutely positioned so that the parent | ||
| element has size. */ | ||
| position: unset; | ||
| } | ||
| .tsf-tooltip-text-wrap { | ||
| background: var(--wp-admin-theme-color); | ||
| flex-shrink: 1; | ||
| max-width: 23em; | ||
| box-shadow: none; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,12 @@ | ||
| .tsf-image-notifications { | ||
| line-height: 1; | ||
| vertical-align: bottom; | ||
|
|
||
| > .tsf-tooltip-wrap { | ||
| /* In gutenberg, this ensures the wrap has the size of its contents, not sure why it’s | ||
stokesman marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| not required in the classic editor. */ | ||
| display: inline-block; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I cannot reproduce this error on Chromium. What browser are you using?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Firefox, Safari and Chrome all show me the same thing. Probably, it’s just not clear what to look for. Here’s a recording that should clarify: social-image-tt-wrap-size.mp4Apparently the popover component in Gutenberg reads the wrap’s rect differently. So, of course, this isn’t required in regular admin screens - the comment could have been better. I suppose this style should move to gbc.css and maybe applied generically to all wraps if it doesn’t mess up any other wraps. |
||
| } | ||
| } | ||
|
|
||
| .tsf-set-image-button.button, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ a.tsf-tooltip-item { | |
| pointer-events: none; | ||
| box-sizing: border-box; | ||
| display: flex; | ||
| flex: 1 1 auto; | ||
| /* flex: 1 1 auto;*/ /* Is this ever in a flex-container? */ | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kept noticing the dev tools telling me this has no effect because the parent element is not a flex container. Perhaps it’s there for a known case but if not I just hoped to remove it and tidy up.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was probably accommodating an old version of the Extension Manager or the SEO Bar pre-TSF v5.0. I'll investigate. Or, most likely, a bad habit of mine where I applied flex normalization everywhere to work around old Safari and Firefox bugs. |
||
| flex-flow: row wrap; | ||
| justify-content: flex-start; | ||
| direction: ltr; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -293,6 +293,79 @@ window.tsfGBC = function( $ ) { | |
| ); | ||
| } | ||
|
|
||
| function GBCTooltip( { element, desc, setTooltip, tt } ) { | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as https://github.com/sybrew/the-seo-framework/pull/748/files#r2549062861: Is this workaround necessary? Is this not a bug upstream? The tooltips work perfectly everywhere else. I'm also not in favor of creating new HTML wrappers, since tt.js is supposed to handle all wrapping autonomously.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might not be clear how much responsibility this wrapper has in GB. The tooltips work everywhere else because they can position themselves to their relative positioned wrap. Once rendered apart from their wraps the positioning task is trickier and this wrapper (react component) handles that. This includes keeping the position updated through scrolling or sizing changes. It’s probably true that ideally the wp-components package could expose a (react) hook, that enables applying the popover positioning system (which is floating-ui) without the Popover component. Speaking of that, before I tried this PR, I tried a branch using floating-ui instead. It’s great in some ways but I doubted it would be favorable because it adds ~20kb. Since the last time I worked on this I happened to remember that the web standard Popover related APIs are coming along nicely. That seems like the ideal thing to base |
||
| const { createElement } = wp.element; | ||
| const { ttNames, ttSelectors } = tt; | ||
|
|
||
| if ( ! desc ) return; | ||
|
|
||
| const __html = `<span class=${ttNames.textWrap}><span class=${ttNames.text}>${desc}</span></span><div class=${ttNames.arrow} style=will-change:left></div>` | ||
|
||
| const hoverItemSuperWrap = element.closest( ttSelectors.superWrap ), | ||
| hoverItemWrap = element.closest( ttSelectors.wrap ) || element.parentElement; | ||
|
|
||
| return createElement( wp.components.Popover, | ||
| { | ||
| // Key to force recreating the popover. Otherwise the popover’s | ||
| // rectangle is stale if an adjacent tooltip was displayed. | ||
| key: desc, | ||
| ref: ( popover ) => { | ||
| if ( ! popover ) return; // The element unmounted. | ||
|
|
||
| setTooltip( popover ); | ||
| // Wait a frame so the rectangle of the popover is available. | ||
| requestAnimationFrame( () => { | ||
| const { x: popoverX } = popover.getBoundingClientRect(); | ||
| const { x: referenceX } = hoverItemWrap.getBoundingClientRect(); | ||
| const relativeX = popoverX - referenceX; | ||
| popover.dataset.adjust = relativeX; | ||
| } ); | ||
| }, | ||
| className: 'tsf-gbc-tooltip-popover', | ||
| anchor: hoverItemSuperWrap || hoverItemWrap, | ||
| animate: false, | ||
| shift: true, | ||
| resize: false, | ||
| variant: 'unstyled', | ||
| placement: 'top', | ||
| inline: true, | ||
| noArrow: false, | ||
| offset: 8, | ||
| }, | ||
| createElement( 'div', { | ||
| className: ttNames.base, | ||
| dangerouslySetInnerHTML: { __html }, | ||
| } ) | ||
| ); | ||
| } | ||
|
|
||
| function overrideTooltip( base ) { | ||
| const rootNode = document.createElement( 'div' ); | ||
| rootNode.classList.add( 'tsf-gbc-tooltip-root' ); | ||
| const root = wp.element.createRoot( | ||
| document.body.appendChild( rootNode ) | ||
| ); | ||
| base.ttSelectors.arrow = '.components-popover__arrow'; | ||
| return { | ||
| _renderTooltip: async ( _event, element, desc ) => { | ||
| element.dataset.hasTooltip = 1; | ||
| const tooltipPromise = new Promise( ( resolve ) => { | ||
| const setTooltip = ( tooltip ) => resolve( tooltip ); | ||
| root.render( | ||
| wp.element.createElement( | ||
| GBCTooltip, | ||
| { element, desc, setTooltip, tt: base } | ||
| ) | ||
| ); | ||
| } ); | ||
| return await tooltipPromise; | ||
| }, | ||
| removeTooltip: ( element ) => { | ||
| root.render( null ); | ||
| base.removeTooltip( element ); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Initializes Gutenberg's compatibility and dispatches event hooks. | ||
| * | ||
|
|
@@ -309,6 +382,9 @@ window.tsfGBC = function( $ ) { | |
| subscribe( saveDispatcher ); | ||
|
|
||
| document.dispatchEvent( new CustomEvent( 'tsf-subscribed-to-gutenberg' ) ); | ||
| document.dispatchEvent( | ||
| new CustomEvent( 'tsf-gutenberg-tt', { detail: overrideTooltip } ) | ||
| ); | ||
|
Comment on lines
+385
to
+387
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using an event for this just seemed nice to not add publicly accessible members though of course it’s still public 🤷. |
||
| } | ||
|
|
||
| return Object.assign( { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -687,9 +687,10 @@ window.tsfMedia = function () { | |
|
|
||
| if ( preview ) { | ||
| if ( success ) { | ||
| // 250px is the max width for tooltips; we subtract 24 for padding, and 1 for subpixel rounding errors. | ||
| // We set min-height and width as that will prevent jumping. Also, those are the absolute-minimum for sharing/schema images. | ||
| imageObject.style = "max-width:225px;max-height:225px;min-width:60px;min-height:60px;border-radius:3px;display:block;"; | ||
| imageObject.style = "max-width:100%;max-height:100%;object-fit:contain;aspect-ratio:1.91;height:auto;border-radius:3px;display:block;"; | ||
| // Add dimensions to prevent jumping. | ||
| imageObject.width = imageObject.naturalWidth; | ||
| imageObject.height = imageObject.naturalHeight; | ||
|
Comment on lines
+690
to
+693
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding the width and height attributes seems to be what fixes the tooltip placement even outside of gutenberg. I’d be glad to extract those to a separate PR if you’d like (or in case this PR isn’t favored). The style attribute changes are for a few reasons:
Looking at this again maybe the
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To consider if min-width/min-height is still necessary, we need to test changing images while keeping the tooltip rendered. i.e., Hover the cursor over the image tooltip, then type/paste a new Social Image URL (the example is incorrect since we get an error, but hopefully illustrates the point). ...why isn't the GIF looping?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It’s looking stable in my testing: Gutenberg: social-image-jump-test.mp4Category edit: social-image-category-jump-test.mp4 |
||
| preview.dataset.desc = imageObject.outerHTML; | ||
|
|
||
| showElement( preview ); | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tangential note: I don’t think 'react' was needed in the deps. It’s certainly not now that 'wp-element' is in there.