Polish external link, avoid CSS bleed. #61646
Closed
Conversation
This file contains 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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +12 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
Iirc this was intentionally left when we switched the icon in order to preserve backwards compatibility with plugins that styled the icon. I think ideally plugins are updated to integrate with the refreshed component, rather than updating the component to account for specific plugins. I may be missing a nuance though, so cc @WordPress/gutenberg-components for more thoughts. |
|
To clarify, the CSS in the screenshot exists in the editor only when the Jetpack plugin is active. It cannot be found in the codebase as far as my skills allowed me to search. |
|
Yup, that's my point. If we add further specificity then we'll 'break' any plugins that target the icon with css currently. In other words; shouldn't this be fixed in Jetpack by removing the component style overrides? |
I'm not following. Right now Jetpack breaks the core component. I would agree this should be fixed in Jetpack, though I'd also suggest that it seems like Shadow DOM was created for this specific purpose, to insulate componentry from unintentional inheritance. On my end this is not a strong opinion, but it seems like it would be up to Jetpack to follow best practices here, and for the components to ensure they are not affected by bleed. |
|
So long as this change doesn't affect any other plugins that target |
t-hamano
reviewed
May 14, 2024
There was a problem hiding this comment.
I tried to find out why Jetpack is adding such styles. Here is the code:
Originally, Gutenberg had an issue where Emotion-based CSS was not being applied within the editor canvas, so this appears to be a temporary fix added on the plugin side. This problem was fixed by #31010 about three years ago.
Considering this, I think it seems ideal to remove this code on the Jetpack side.
It is difficult to predict how the CSS of all components in Gutenberg will be affected by the plugin, not just this component. Of course, if a destructive change is made to the officially published API, it is necessary to maintain backward compatibility, but when it comes to CSS, I think it is the responsibility of the plugin to respond to the change.
|
Great investigative work, thanks for looking. So just to be sure I'm understanding things right, are you recommending that I report this to the Jetpack team to have it fixed there, and only there, i.e. close this PR? Happy to, just making sure — these seem like properties worth giving extra specificity, but perhaps not that worth it if it diverges in terms of code style. |
Yes, I consider it ideal. |
|
Excellent, thanks for looking. I'll do that. |
|
To close the loop on this, that extra CSS will be removed here: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
The ExternalLink component is subject to CSS bleed from plugins that also style this same classname.
This PR increases specificity and unstyles some pieces that break the core component. It also removes the font-weight so that the weight of the glyph is the same as adjacent text:
The font weight change is mainly motivated by leveraging the fact that the unicode glyph is the same font and size as the surrounding text, so by keeping the weight in sync gels with that.
Testing Instructions