Skip to content
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

Update Link Control labels to use gray-900 #55867

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

joanrodas
Copy link
Contributor

What?

Update Link Control labels to use correct gray color ($gray-900)

Fixes #54589

PR in WordCamp Madrid 2023 Contributor Day with @Albert-Sirvelia @AlexBrea

Why?

Labels in Link Control are not using the correct gray color ($gray-900).

How?

Modifying the style.scss of the Link Control component and changing the color of the labels

Testing Instructions

  1. Open a post or page.
  2. Insert a link
  3. Edit link and open advance settings

Screenshots or screencast

Link Control

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Nov 5, 2023
Copy link

github-actions bot commented Nov 5, 2023

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @joanrodas! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@cbravobernal cbravobernal added [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Type] Code Quality Issues or PRs that relate to code quality labels Nov 5, 2023
@getdave getdave added [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) and removed [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") labels Nov 7, 2023
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this and looked at all the labels I could find to check they were using $gray-900; (aka #1e1e1e) and they were.

Thank you for the PR 👏

@getdave getdave merged commit 659c960 into WordPress:trunk Nov 7, 2023
53 of 54 checks passed
@github-actions github-actions bot added this to the Gutenberg 17.1 milestone Nov 7, 2023
@richtabor
Copy link
Member

@ciampo Is this something we can fix from the modal itself? This is the case throughout the editor where $gray-900 should be used as the default text color in modals.

CleanShot 2023-11-13 at 11 50 19
CleanShot 2023-11-13 at 11 50 44

@getdave
Copy link
Contributor

getdave commented Nov 13, 2023

Bear in mind that LinkUI does not use <Modal> (it uses Popover) 🙇

@richtabor
Copy link
Member

I'll write up a separate issue. 😅

@ciampo
Copy link
Contributor

ciampo commented Nov 15, 2023

Is this something we can fix from the modal itself? This is the case throughout the editor where $gray-900 should be used as the default text color in modals.

I don't think that Modal or Popover are setting any specific CSS color, it's more down to the individual components rendered inside those modals/popovers.

Therefore, more than in the specific component, this is something that we should address holistically at the package level.

We could add CSS color rules to every component displaying text — and especially when Theming is available, all components will be able to display the correct text color automatically based on the theme.

The "drawback" of this approach is that we would stop relying on the CSS cascade — meaning that, if a consumer of the components package wanted to use a different text color for any reason, they would have to override the CSS color for every component used (instead of declaring it once and forget it).

Comment on lines +64 to +66
.components-base-control__label {
color: $gray-900;
}
Copy link
Contributor

@ciampo ciampo Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding style overrides is usually not the best way to fix these issues:

  • If BaseControl's default label color is wrong, maybe the fix should happen directly in BaseControl (see the component in isolation here)
  • Otherwise, if BaseControl's default label color is correct, but the color is wrong specifically for LinkControl, then we should find which other CSS rule is changing the color, and work on a fix at that level
  • We shouldn't be using internal .components- classnames, since those are meant to be implementation details of those components.

Any chance we can try to solve this issue in an alternative way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Code Quality Issues or PRs that relate to code quality
5 participants