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

Removing obsolete currentColor CSS value #14119

Merged
merged 5 commits into from
Apr 10, 2019
Merged

Removing obsolete currentColor CSS value #14119

merged 5 commits into from
Apr 10, 2019

Conversation

webmandesign
Copy link
Contributor

Fixes #12328

@webmandesign
Copy link
Contributor Author

I've found some additional questionable usages of currentColor, possibly also need an update:

  1. In packages/block-library/src/table/editor.scss file on line 30 the currentColor was recently changed to value of $black. I'm not sure if this is necessary. I suggest removing $black as in my original PR Removing redundant currentColor CSS value #12329 so we use text color instead of forcing a specific black color on borders. Can I go ahead and change this, what do you think?

  2. OPTIONAL: In packages/editor/src/components/inserter-list-item/style.scss file the currentColor values could be replaced also with inherit.

  3. OPTIONAL: In packages/components/src/button/style.scss file the currentColor values could be replaced also with inherit.

What do you say guys? @jasmussen @kjellr @gziolo

PS: I'm really sorry I haven't made it for 5.1 release... :(

@jasmussen
Copy link
Contributor

Thanks so much for your work. Don't feel bad that this didn't make 5.1, it will land in WordPress eventually, and even before that it will land in the plugin release, so you can start using it so long as you have the plugin enabled. Your contributions are very valuable, and in its current state, it's 👍 👍 ready to go!

However since you also wrote some good thoughts around additional changes, those might be good to get in, and then get a final review.

Specificallhy, options 2 and 3 on your list seem fine to me to go ahead and fix.

For option 1, let's get some feedback from @talldan, which I believe made some changes to that file recently. Do you see any reason we can't replace the $black variable with, well, nothing?

@webmandesign
Copy link
Contributor Author

Thanks @jasmussen Re 2 and 3, the thing is that it's just about what you guys prefer to use there. Both currentColor and inherit should return the same value and both have sufficient browser support. I personally tend to use inherit in such cases. So, if that's really fine with you too, I will update those values.

I'll wait for @talldan's feedback to proceed with removing $black.

@jasmussen
Copy link
Contributor

Yep, inherit if the behavior is essentially the same, sounds good to me.

@talldan
Copy link
Contributor

talldan commented Feb 28, 2019

Hi @webmandesign - the change for the table block looks good 👍.

I don't think my change to $black should have made it in 🤦‍♂️. Originally my PR introduced the ability to change the color of the text, which is why I changed it to $black, but then that functionality was dropped from the PR. It looks as though this line wasn't reverted.

@webmandesign
Copy link
Contributor Author

@talldan Thank you for info. I've went ahead and removed the $black from table border too.

All is fixed now.

The only instances left with currentColor in styles are fill: currentColor;, which is perfectly correct and don't require any update.

@webmandesign
Copy link
Contributor Author

Hey guys, any update on this please?

@jasmussen
Copy link
Contributor

Thanks for your work and patience. I'm going to put this in a milestone so it's guaranteed a few eyes soon. In the mean time, it needs a good rebase to solve the conflicts.

@jasmussen jasmussen added this to the 5.4 (Gutenberg) milestone Mar 20, 2019
@webmandesign
Copy link
Contributor Author

Thanks @jasmussen I'll do my best to rebase, though I don't have much luck with them ;)

@jasmussen
Copy link
Contributor

If it's easier to start a new branch with the small changes discussed here, that's okay too, just ping me on the new branch and I'll milestone that — sorry about the delay.

…urrentcolor-css-value

# Conflicts:
#	packages/block-editor/src/components/inserter-list-item/style.scss
@webmandesign
Copy link
Contributor Author

Hi @jasmussen, I'm sorry for delay, but it seems I was able to match the commits with current master branch. Please double check, just in case, but from my point of view everything seems to be fine. Thank you!

@jasmussen jasmussen self-requested a review March 28, 2019 08:41
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Thank you!

@talldan
Copy link
Contributor

talldan commented Apr 10, 2019

Merging this since it has the 👍 of approval.

@talldan talldan merged commit a76a702 into WordPress:master Apr 10, 2019
@webmandesign
Copy link
Contributor Author

Thank you!

mchowning pushed a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019
* Removing obsolete `currentColor` value

* Removing obsolete `currentColor` from CSS

* Removing obsolete `$black` color so table border inherits text color

* Changing `currentColor` value to `inherit` where appropriate
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.

4 participants