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: removed purple site title link color in dusk theme #610

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

troychaplin
Copy link
Contributor

Description

This addresses #604. Removed paragraph link style that impacts site title and compared to other styles, which do not seem to have this link style in place. Some screenshots of how link in a para is treated with just an underline, no specific color treatment.

Screenshots

example-2 example-3 example-4 example-1

Copy link

github-actions bot commented Oct 21, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: troychaplin <[email protected]>
Co-authored-by: carolinan <[email protected]>
Co-authored-by: juanfra <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

github-actions bot commented Oct 21, 2024

Preview changes

You can preview these changes by following the link below:

I will update this comment with the latest preview links as you push more changes to this PR.

Note

The preview sites are created using WordPress Playground. You can add content, edit settings, and test the themes as you would on a real site, but please note that changes are not saved between sessions.

@carolinan
Copy link
Contributor

This change affects Dusk, but the screenshots are of the other variations.

In the design, the links are supposed to be purple, so this style should not be removed.
https://www.figma.com/design/dzGCSntVch4EQdVERTqyVK/Twenty-Twenty-Five?node-id=77-338&node-type=frame&t=wE2ugqqAeMGWDKJ3-0

Does setting the correct color on styles > Blocks > core/site-title > elements > link not work?

@troychaplin
Copy link
Contributor Author

This change affects Dusk, but the screenshots are of the other variations.

In the design, the links are supposed to be purple, so this style should not be removed. https://www.figma.com/design/dzGCSntVch4EQdVERTqyVK/Twenty-Twenty-Five?node-id=77-338&node-type=frame&t=wE2ugqqAeMGWDKJ3-0

Does setting the correct color on styles > Blocks > core/site-title > elements > link not work?

Oh, then I’ll look at this again today

@juanfra
Copy link
Member

juanfra commented Oct 24, 2024

@troychaplin please let us know if you still have time to iterate on this. Thanks 🙏

@troychaplin
Copy link
Contributor Author

@troychaplin please let us know if you still have time to iterate on this. Thanks 🙏

Working on it. Can't figure it out, probably missing something easy here 😕

@carolinan
Copy link
Contributor

carolinan commented Oct 25, 2024

The site title can't override the CSS that WP adds to the link color to the paragraph.

In Dusk, can you try removing the link color from the site title, and adding this directly under styles:
"css": "p.wp-block-site-title:not(.has-link-color) a { color: var(--wp--preset--color--accent-3);}",

That way it overrides the link style on the p element, and if the user adds a color in the block setting, that setting works.

@troychaplin
Copy link
Contributor Author

The site title can't override the CSS that WP adds to the link color to the paragraph.

In Dusk, can you try removing the link color from the site title, and adding this directly under styles: "css": "p.wp-block-site-title:not(.has-link-color) a { color: var(--wp--preset--color--accent-3);}",

That way it overrides the link style on the p element, and if the user adds a color in the block setting, that setting works.

Thank you! I’ll get back to this one shortly!

@juanfra
Copy link
Member

juanfra commented Oct 25, 2024

@carolinan another reason to try to get the class in the paragraph block 😅

@troychaplin
Copy link
Contributor Author

@carolinan thank you, this worked well and changes have been pushed

@troychaplin troychaplin linked an issue Oct 25, 2024 that may be closed by this pull request
@troychaplin troychaplin changed the title update: removed para link style that impact site title update: removed purple site title link color in dusk theme Oct 25, 2024
@carolinan
Copy link
Contributor

There seems to be several changes to the json files that are unrelated to changing the link color on the site title?

@troychaplin
Copy link
Contributor Author

There seems to be several changes to the json files that are unrelated to changing the link color on the site title?

I just reviewed and one change to colors/dusk was reverted. Other changes to styles/dusk are coming from a merge from trunk into the branch. The key change for this on it around site-title and removing the link element and adding custom css

@carolinan
Copy link
Contributor

I don't understand. The underline changes are all unrelated.

@troychaplin
Copy link
Contributor Author

I don't understand. The underline changes are all unrelated.

I went back to a clean branch to test this out, and either I have something wrong in my local setup, or this issue has been resolved with changes elsewhere. I can't recreate the issue in trunk.

@juanfra
Copy link
Member

juanfra commented Oct 28, 2024

@troychaplin I believe what Carolina is sharing is that if you go to the files changed in this PR, you can see that the submission includes different changes that are unrelated to the problem described in the issue:

  1. Lines 90-97
  2. Lines 113-120
  3. Lines 161-167
  4. Lines 256-263

What would fix the issue is the change in line 180:

"css": "p.wp-block-site-title:not(.has-link-color) a { color: var(--wp--preset--color--accent-3);}",

@troychaplin
Copy link
Contributor Author

@troychaplin I believe what Carolina is sharing is that if you go to the files changed in this PR, you can see that the submission includes different changes that are unrelated to the problem described in the issue:

1. Lines [90-97](https://github.com/WordPress/twentytwentyfive/pull/610/files#diff-424d7ce7d0eba2b0c09e49c130cb86989281e8862e869ff83ab786798659bf11R90-R97)

2. Lines [113-120](https://github.com/WordPress/twentytwentyfive/pull/610/files#diff-424d7ce7d0eba2b0c09e49c130cb86989281e8862e869ff83ab786798659bf11R113-R120)

3. Lines [161-167](https://github.com/WordPress/twentytwentyfive/pull/610/files#diff-424d7ce7d0eba2b0c09e49c130cb86989281e8862e869ff83ab786798659bf11L161-L167)

4. Lines [256-263](https://github.com/WordPress/twentytwentyfive/pull/610/files#diff-424d7ce7d0eba2b0c09e49c130cb86989281e8862e869ff83ab786798659bf11R256-R263)

What would fix the issue is the change in line 180:

"css": "p.wp-block-site-title:not(.has-link-color) a { color: var(--wp--preset--color--accent-3);}",

Yeah, I'm not sure where those came from. I assumed it was a merge from trunk but no. I went to create a new branch to test and fix properly, but I cannot recreate the purple in site title locally anymore, and I'm not sure if I'm having some odd issues locally, or if this got fixed through CSS work in other areas.

@juanfra
Copy link
Member

juanfra commented Oct 28, 2024

Maybe those come from a merge conflict when updating trunk?

I can still see the issue when checking on trunk.

@troychaplin
Copy link
Contributor Author

Maybe those come from a merge conflict when updating trunk?

I can still see the issue when checking on trunk.

Maybe. Might be best to just close this PR out and I'll submit a new one with only the css changes needed

@troychaplin
Copy link
Contributor Author

Ok, so I've reverted back those questionable changes, and have figured out my local issues so I can see these changes properly. Unfortunately adding inline css is not the ultimate fix, not sure what else needs to be done.

I did notice that Sunrise has specific link colours that are not the same as what is being applied to site title, I'll look at differences and see if I can figure out where the issue lies.

@carolinan
Copy link
Contributor

Unfortunately adding inline css is not the ultimate fix, not sure what else needs to be done.

There is nothing else the theme can do under these conditions, since the bug is in the editor.

@carolinan
Copy link
Contributor

The dusk color preset still uses the old styles, it needs to be updated too.

@troychaplin
Copy link
Contributor Author

The dusk color preset still uses the old styles, it needs to be updated too.

Sorry, haven't had a chance to come back to TT5 lately. So even after the change in the color preset the title remains purple, as you noted. Is this blocked until the issue in the editor relating to classes on the paragraph block is fixed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dusk: Site title is colored
3 participants