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

Blocks: Refactor the block toolbar component and move it to its own component #2885

Merged
merged 2 commits into from Oct 6, 2017

Conversation

Projects
None yet
3 participants
@youknowriad
Contributor

youknowriad commented Oct 5, 2017

This refactors the block toolbar to its own component. It has several goals:

  • Having a lighter VisualEditorBlock component (and style)
  • Better directory structure (BlockToolbar same level as BlockMover, BlockInspector...)
  • Prepare the toolbar for future enhancements (arrow navigation, fixed toolbar?)

This also fixes some styling glitches on mobile (scrollbar always appearing in the toolbar)

Testing instructions

  • Check the docked block toolbar still works as expected in Mobile and in Desktop

@youknowriad youknowriad self-assigned this Oct 5, 2017

@youknowriad youknowriad requested review from jasmussen and aduth Oct 5, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 5, 2017

Codecov Report

Merging #2885 into master will decrease coverage by 0.09%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2885     +/-   ##
=========================================
- Coverage   33.95%   33.85%   -0.1%     
=========================================
  Files         191      192      +1     
  Lines        5690     5706     +16     
  Branches      997     1000      +3     
=========================================
  Hits         1932     1932             
- Misses       3180     3193     +13     
- Partials      578      581      +3
Impacted Files Coverage Δ
editor/block-toolbar/index.js 0% <0%> (ø)
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e84bec...42809af. Read the comment docs.

codecov bot commented Oct 5, 2017

Codecov Report

Merging #2885 into master will decrease coverage by 0.09%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2885     +/-   ##
=========================================
- Coverage   33.95%   33.85%   -0.1%     
=========================================
  Files         191      192      +1     
  Lines        5690     5706     +16     
  Branches      997     1000      +3     
=========================================
  Hits         1932     1932             
- Misses       3180     3193     +13     
- Partials      578      581      +3
Impacted Files Coverage Δ
editor/block-toolbar/index.js 0% <0%> (ø)
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e84bec...42809af. Read the comment docs.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 5, 2017

Contributor

Nice!

This seems to work perfectly except for when blocks are wide or full wide, where there are some issues both at the desktop and mobile breakpoints:

screen shot 2017-10-05 at 12 29 21

screen shot 2017-10-05 at 12 30 05

Contributor

jasmussen commented Oct 5, 2017

Nice!

This seems to work perfectly except for when blocks are wide or full wide, where there are some issues both at the desktop and mobile breakpoints:

screen shot 2017-10-05 at 12 29 21

screen shot 2017-10-05 at 12 30 05

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 5, 2017

Contributor

Good catch @jasmussen it should be better now

Contributor

youknowriad commented Oct 5, 2017

Good catch @jasmussen it should be better now

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 5, 2017

Contributor

Confirmed, seems to work perfectly now, nice fix!

A quick glitch I noticed, probably separate to this:

screen shot 2017-10-05 at 12 59 44

Looks like the Cover Image text is at a higher z index than the toolbar. We can look at this separately, but 👍 👍

Contributor

jasmussen commented Oct 5, 2017

Confirmed, seems to work perfectly now, nice fix!

A quick glitch I noticed, probably separate to this:

screen shot 2017-10-05 at 12 59 44

Looks like the Cover Image text is at a higher z index than the toolbar. We can look at this separately, but 👍 👍

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 5, 2017

Contributor

@jasmussen yep, confirmed the bug is also on master, z-index tweaking is hard :)

Contributor

youknowriad commented Oct 5, 2017

@jasmussen yep, confirmed the bug is also on master, z-index tweaking is hard :)

@aduth

aduth approved these changes Oct 5, 2017

  • Having a lighter VisualEditorBlock component (and style)

❤️

@youknowriad youknowriad merged commit 3a9048c into master Oct 6, 2017

3 checks passed

codecov/project 33.85% (-0.1%) compared to 9e84bec
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/block-toolbar branch Oct 6, 2017

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