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

Introduce a ResizableBox component #10347

Merged
merged 7 commits into from Oct 7, 2018

Conversation

Projects
None yet
3 participants
@chrisvanpatten
Contributor

chrisvanpatten commented Oct 4, 2018

This is a follow-up to #10331 and fixes #10343.

This PR…

  • introduces a ResizableBox component which wraps the re-resizable package to remove some code duplication
  • introduces global/shared classes for resize handles
  • removes the mixins introduced in #10331, and thus the code duplication (which had existed for a while before that)
  • updates core/image and core/spacer to use this new component

I plan to flesh out the docs, but want to make sure I'm on the right track with this first! Still want to know if I'm on track, but I added additional docs anyway :)

Testing

  • Insert image and spacer blocks
  • Verify there are no visual or functional regressions at various alignments and configurations

@chrisvanpatten chrisvanpatten requested review from tofumatt and WordPress/gutenberg-core Oct 4, 2018

@tofumatt tofumatt added this to the 4.1 milestone Oct 4, 2018

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 4, 2018

Member

Thanks for jumping on the patch for this! 😄

I'l try to take a look this week, but I might have to wait a bit until after we clear out the 4.0 milestone. 😅

Member

tofumatt commented Oct 4, 2018

Thanks for jumping on the patch for this! 😄

I'l try to take a look this week, but I might have to wait a bit until after we clear out the 4.0 milestone. 😅

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 5, 2018

Contributor

@tofumatt Also should note that if you want to take this off your plate, feel free to unassign yourself from the review! I requested you because you had reviewed #10331 but definitely don't feel obligated to review this too :) I know you have a lot going on!

Contributor

chrisvanpatten commented Oct 5, 2018

@tofumatt Also should note that if you want to take this off your plate, feel free to unassign yourself from the review! I requested you because you had reviewed #10331 but definitely don't feel obligated to review this too :) I know you have a lot going on!

@tofumatt

I dig it! I think we need to add to the components changelog to mention the new component we've introduced. 🤔

After that I think this is good! Thanks for doing this! ❤️

Show outdated Hide outdated packages/components/src/resizable-box/README.md Outdated
@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 5, 2018

Contributor

@tofumatt Clarified the docs and added to the changelog. Might need a check from @gziolo to be sure I updated the changelog correctly (although I do believe it's right; I checked the file history to see how it had been handled previously).

Contributor

chrisvanpatten commented Oct 5, 2018

@tofumatt Clarified the docs and added to the changelog. Might need a check from @gziolo to be sure I updated the changelog correctly (although I do believe it's right; I checked the file history to see how it had been handled previously).

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 5, 2018

Member

I think the test failures are unrelated, see: #10364.

Once that's merged you should probably rebase and then flag me for another review... but I think this is all good 👍

Member

tofumatt commented Oct 5, 2018

I think the test failures are unrelated, see: #10364.

Once that's merged you should probably rebase and then flag me for another review... but I think this is all good 👍

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 5, 2018

Contributor

@tofumatt looks like Travis is still failing for the Undo and Split/Merge tests, which is likely unrelated.

Contributor

chrisvanpatten commented Oct 5, 2018

@tofumatt looks like Travis is still failing for the Undo and Split/Merge tests, which is likely unrelated.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 5, 2018

Member

Yup, that one is an "expected" intermittent failure 😅 (restarted)

Member

tofumatt commented Oct 5, 2018

Yup, that one is an "expected" intermittent failure 😅 (restarted)

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 5, 2018

Member

It keeps failing, maybe something else is up? 😢

Member

tofumatt commented Oct 5, 2018

It keeps failing, maybe something else is up? 😢

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 5, 2018

Contributor

@tofumatt I’ll try the tests locally. It’s strange it only happens in the one environment in Travis…

Contributor

chrisvanpatten commented Oct 5, 2018

@tofumatt I’ll try the tests locally. It’s strange it only happens in the one environment in Travis…

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 6, 2018

Contributor

I got the local test environment set up and I can't get the undo test working there (split/merge seems good now)… but I also can't get the undo test working in master so it seems like just a sporadic/unpredictable/unrelated failure.

Contributor

chrisvanpatten commented Oct 6, 2018

I got the local test environment set up and I can't get the undo test working there (split/merge seems good now)… but I also can't get the undo test working in master so it seems like just a sporadic/unpredictable/unrelated failure.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 6, 2018

Member

Ten four, I've kicked the test server again and hopefully it'll work done 🤷‍♂️

Member

tofumatt commented Oct 6, 2018

Ten four, I've kicked the test server again and hopefully it'll work done 🤷‍♂️

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 6, 2018

Contributor

Strangely I can't "pass" the undo test in master (or this branch) even when I manually follow the instructions (typing in three lines and hitting undo six times). It actually takes me 7 or 8 undos before I'm back to a disabled undo state.

I'm wondering if this might actually be a bug in master right now? I can't reproduce it in 3.9.

Contributor

chrisvanpatten commented Oct 6, 2018

Strangely I can't "pass" the undo test in master (or this branch) even when I manually follow the instructions (typing in three lines and hitting undo six times). It actually takes me 7 or 8 undos before I'm back to a disabled undo state.

I'm wondering if this might actually be a bug in master right now? I can't reproduce it in 3.9.

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 6, 2018

Contributor

lol okay just as I type that the test finally passes ¯_(ツ)_/¯

@tofumatt I think we're finally clear on this… phew…

(Also should it still be held until 4.1? Will there still be a 4.1 at this point…?)

Contributor

chrisvanpatten commented Oct 6, 2018

lol okay just as I type that the test finally passes ¯_(ツ)_/¯

@tofumatt I think we're finally clear on this… phew…

(Also should it still be held until 4.1? Will there still be a 4.1 at this point…?)

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 6, 2018

Member
Member

tofumatt commented Oct 6, 2018

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 6, 2018

Member

In general I’m for merging as much stuff as possible and this is pretty minimal. I’ll have a look tomorrow, been a bit distracted by travel today.

There will be a 4.1 by the way! In general Gutenberg development (including releases) will continue on GitHub, so while I’m not exactly sure how things will work after WordPress 5.0, I think the general idea will be that certain releases of WordPress will pull in a certain release of Gutenberg, if that makes sense.

Member

tofumatt commented Oct 6, 2018

In general I’m for merging as much stuff as possible and this is pretty minimal. I’ll have a look tomorrow, been a bit distracted by travel today.

There will be a 4.1 by the way! In general Gutenberg development (including releases) will continue on GitHub, so while I’m not exactly sure how things will work after WordPress 5.0, I think the general idea will be that certain releases of WordPress will pull in a certain release of Gutenberg, if that makes sense.

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 7, 2018

Contributor

@tofumatt totally fair! Travel can’t can knock the wind out of you.

I’m going to try a git blame bisect to see if I can figure out when the undo behaviour changed and if I can figure it out I’ll open a new issue.

Contributor

chrisvanpatten commented Oct 7, 2018

@tofumatt totally fair! Travel can’t can knock the wind out of you.

I’m going to try a git blame bisect to see if I can figure out when the undo behaviour changed and if I can figure it out I’ll open a new issue.

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 7, 2018

Contributor

@tofumatt just opened #10380 which tracks the undo issue. It looks related to the rich text state structure changes, which makes sense.

Contributor

chrisvanpatten commented Oct 7, 2018

@tofumatt just opened #10380 which tracks the undo issue. It looks related to the rich text state structure changes, which makes sense.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 7, 2018

Member
Member

tofumatt commented Oct 7, 2018

@tofumatt

I dig it 👍

@chrisvanpatten chrisvanpatten merged commit eafd620 into WordPress:master Oct 7, 2018

2 checks passed

codecov/project 49.3% (-0.01%) compared to 86dad5f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chrisvanpatten chrisvanpatten modified the milestones: 4.1, 4.0 Oct 7, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Oct 10, 2018

Member

This introduced changes when running npm install on master because the package-lock.json file wasn't updated.

Member

aduth commented Oct 10, 2018

This introduced changes when running npm install on master because the package-lock.json file wasn't updated.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth
Member

aduth commented Oct 10, 2018

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 10, 2018

Contributor

Ughhh I'm really sorry @aduth 😭 I should have caught that…

Contributor

chrisvanpatten commented Oct 10, 2018

Ughhh I'm really sorry @aduth 😭 I should have caught that…

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