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

Components: remove color() utility function, use values directly #31661

Merged
merged 5 commits into from
May 12, 2021
Merged

Components: remove color() utility function, use values directly #31661

merged 5 commits into from
May 12, 2021

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented May 10, 2021

Description

Remove the color() utility function, and instead access the color values directly.

This is similar to what has been done in #31646 for the config() function, and is part of what highlighted in #30503

How has this been tested?

  • Make sure that the project builds correctly
  • Make sure that the affected components look and behave the same as on trunk (e.g. by using Storybook)

Screenshots

N/A

Types of changes

Refactor / cleanup

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • N/A I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • N/A I've included developer documentation if appropriate.
  • N/A I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Code looks good and this tests well for me in Storybook 👍

@@ -15,6 +15,7 @@
### Internal

- `Flex`, `FlexBlock`, and `FlexItem` components have been re-written from the ground up ([#31297](https://github.com/WordPress/gutenberg/pull/31297)).
- `color()` utility function has been removed, in favour of accessing color values directly ([#31661](https://github.com/WordPress/gutenberg/pull/31661))
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we're mixing American and British English when using color (US) but favour (UK). I guess we should stick to the American version as it's the one used in web terms.

Suggested change
- `color()` utility function has been removed, in favour of accessing color values directly ([#31661](https://github.com/WordPress/gutenberg/pull/31661))
- `color()` utility function has been removed, in favor of accessing color values directly ([#31661](https://github.com/WordPress/gutenberg/pull/31661))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted! I normally write in British English, and so this happens more often than I'd like 😅

Actually, in light of @diegohaz 's comment on a very similar PR, should I actually just delete this changelog entry?

Copy link
Member

Choose a reason for hiding this comment

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

Yup, agree that changes in non-exposed internals don't need to be documented here 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in dd4b976

@ciampo
Copy link
Contributor Author

ciampo commented May 12, 2021

Code looks good and this tests well for me in Storybook 👍

Thank you for your review, @tyxla !

Since I don't have repo privileges yet, would you mind merging this PR? Thank you!

@tyxla
Copy link
Member

tyxla commented May 12, 2021

Since I don't have repo privileges yet, would you mind merging this PR? Thank you!

Sure thing! Thanks for your work here 🙌

@tyxla tyxla merged commit 4b0f0f0 into WordPress:trunk May 12, 2021
@github-actions github-actions bot added this to the Gutenberg 10.7 milestone May 12, 2021
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.

2 participants