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

Enhancements to the color mechanism in accordance with the latest changes #7986

Merged
merged 1 commit into from Aug 20, 2018

Conversation

@jorgefilipecosta
Member

jorgefilipecosta commented Jul 16, 2018

BREAKING CHANGE (with back compatibility).

The color mechanism suffered many changes since it was created. Namely the fact that now the identified for colors is not the name but a slug.
This PR applies some changes to improve the code quality.
It also removes the deprecation logic that should be removed in this version.

It applies some changes to make sure withColors now passed all the information contained in the color object as set by the themes to the blocks using withColors HOC.

The breaking change is that now the color code passed in objects by withColors is named as "color" instead of "value".
That change was made because of two reasons:

  • To be consistent with the withFontSizes HOC which name the font size value as size.
  • To be consistent with the color object that themes set, which names the color code as color:

How has this been tested?

Verify the colors of the button and paragraph continue to work as before.

@jorgefilipecosta jorgefilipecosta self-assigned this Jul 16, 2018

@jorgefilipecosta jorgefilipecosta added this to In Progress in API freeze via automation Jul 23, 2018

@jorgefilipecosta jorgefilipecosta changed the title from chore: Enhancements to the color mechanism in accordance with the latest changes to Enhancements to the color mechanism in accordance with the latest changes Jul 27, 2018

@jorgefilipecosta jorgefilipecosta added this to the 3.4 milestone Jul 30, 2018

@aduth

I don't think this is as high priority as it's been flagged to be. Partly because it's not a documented utility† and its usage is likely very limited.

† Why isn't it documented?

@pento pento modified the milestones: 3.4, 3.5 Jul 31, 2018

@jorgefilipecosta jorgefilipecosta removed this from the 3.5 milestone Aug 2, 2018

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Aug 2, 2018

Member

I don't think this is as high priority as it's been flagged to be. Partly because it's not a documented utility† and its usage is likely very limited.

Regarding the priority, I just made it a high priority because it was changing the API, but yes as it was not documented I agree the prioriy could be lower and I corrected it.

† Why isn't it documented?

We had a pending PR that adds the documentation to this API. It was created before this PR, so I will update it and say it depends on this one being merged.

Member

jorgefilipecosta commented Aug 2, 2018

I don't think this is as high priority as it's been flagged to be. Partly because it's not a documented utility† and its usage is likely very limited.

Regarding the priority, I just made it a high priority because it was changing the API, but yes as it was not documented I agree the prioriy could be lower and I corrected it.

† Why isn't it documented?

We had a pending PR that adds the documentation to this API. It was created before this PR, so I will update it and say it depends on this one being merged.

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Aug 2, 2018

Contributor

I also had offered to contribute to the color docs but life got in the way as it often does 😓

Contributor

chrisvanpatten commented Aug 2, 2018

I also had offered to contribute to the color docs but life got in the way as it often does 😓

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Aug 2, 2018

Member

Hi @chrisvanpatten no problem I will update the docs PR to match this PR, you can still improve the docs and branch from it after the PR is updated.

Member

jorgefilipecosta commented Aug 2, 2018

Hi @chrisvanpatten no problem I will update the docs PR to match this PR, you can still improve the docs and branch from it after the PR is updated.

@gziolo

This PR need to be rebased.

I also left a few comments.

Show outdated Hide outdated core-blocks/button/edit.js
Show outdated Hide outdated docs/reference/deprecated.md
Show outdated Hide outdated packages/editor/src/components/colors/with-colors.js
Show outdated Hide outdated docs/reference/deprecated.md

@gziolo gziolo added this to the 3.6 milestone Aug 13, 2018

@youknowriad youknowriad modified the milestones: 3.6, 3.7 Aug 15, 2018

@gziolo

gziolo approved these changes Aug 20, 2018

Themes set the color object with a color property. If we want to use value should themes also pass a value property?

Let's leave it as is for consistency 👍

Please make sure to update deprecation versions before merging this PR.

chore: Update: Some enhancements to the color mechanism to improve co…
…de in accordance with the latest changes.

@jorgefilipecosta jorgefilipecosta merged commit f631337 into master Aug 20, 2018

2 checks passed

codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +9.07% compared to bc4d7a2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

API freeze automation moved this from In Progress to Done Aug 20, 2018

@mtias mtias deleted the update/improve-code-after-latest-with-colors-changes branch Aug 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment