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

Fix Text Color Issue in Table Block in TT1 Theme #8058

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

shail-mehta
Copy link
Member

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@shail-mehta shail-mehta force-pushed the fix/text-color-issue-in-table branch from 700b306 to 923bc34 Compare December 29, 2024 16:04
@shail-mehta shail-mehta marked this pull request as ready for review December 29, 2024 16:17
Copy link

github-actions bot commented Dec 29, 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.

Core Committers: Use this line as a base for the props when committing in SVN:

Props shailu25, wildworks, poena.

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

@shail-mehta shail-mehta changed the title Fix/text color issue in table Fix Text Color Issue in Table Block in TT1 Theme Dec 29, 2024
@t-hamano
Copy link

t-hamano commented Jan 4, 2025

Thanks for the PR!

This PR works as expected, but I'm wondering why the following style even exists in the first place:

table,
.wp-block-table {
	&.is-style-regular .has-background,
	&.is-style-stripes .has-background,
	&.is-style-stripes .has-background thead tr,
	&.is-style-stripes .has-background tfoot tr,
	&.is-style-stripes .has-background tbody tr {
		color: var(--table--has-background-text-color);
	}
}

Because the following two CSS variables both point to #28303d, the cell's text color will not change regardless of whether the table has a background color or is the Stripes style:

  • Cell text color when no background color is applied to the table: var(--global--color-primary) is applied
  • Cell text color when a background color is applied to a table: var(--global--color-dark-gray) is applied

My guess is that removing this style entirely would be the ideal approach. Perhaps this style was needed when the Table block didn't support color.

cc @WordPress/block-themers For additional feedback

@carolinan
Copy link
Contributor

Twenty Twenty-One is not a block theme.

@carolinan
Copy link
Contributor

Please remember that the bundled themes, according to instructions from Matt, must be backwards compatible,
It is not enough to support a version where the block has specific color settings available.

@t-hamano
Copy link

t-hamano commented Jan 4, 2025

Twenty Twenty-One is not a block theme.
It is not enough to support a version where the block has specific color settings available.

I understand this of course.

This style was added in this commit. According to the commit message, this style was added to solve an issue with dark mode. But there is a similar style here, so it looks fine as far as I've tested. Will there be any backward compatibility issues?

according to instructions from Matt

What is "instructions from Matt"? Why is Matt's name being mentioned here? I'm a little confused.

@carolinan
Copy link
Contributor

carolinan commented Jan 4, 2025

I wrote that Twenty Twenty-One is not a block theme because you pinged the block theme group.

Ideally for us as developers, we would only need to support the latest or two latest versions of WordPress. Then all the bundled themes could be refactored to new standards and a large portion of the CSS could be removed.
-Including CSS that may only have been needed before blocks had specific settings.
I am only putting weight to that decision while explaining why it is not enough to only support one version.

@t-hamano
Copy link

t-hamano commented Jan 5, 2025

I wrote that Twenty Twenty-One is not a block theme because you pinged the block theme group.

Sorry, I mistakenly thought this group included all the default theme developers.

Ideally for us as developers, we would only need to support the latest or two latest versions of WordPress.

I strongly agree with this.

I know this is not something we should discuss here, but for example, Twenty Twenty-One supports WP version 5.6 and up. Twenty Ten, surprisingly, supports WordPress version 3.0 and up.

When trying to fix a bug in bundled themes, it seems nearly impossible to test and ensure that it works on all supported WordPress versions.

I have little experience updating bundled themes, but I'm interested to see how you deal with issues like this.

@carolinan
Copy link
Contributor

It is difficult, especially when trying to test on versions that require PHP 5.x 😰

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.

3 participants