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

Show transformations in the ellipsis menu #3458

Merged
merged 3 commits into from Dec 14, 2017

Conversation

@jorgefilipecosta
Member

jorgefilipecosta commented Nov 13, 2017

Description

This PR aims to close #3434 by adding the transformations to the block side menu.

The getPossibleBlockTransformations function was extracted from BlockSwitcher component as it was in order for us to be able to use this logic outside of BlockSwitcher component.

How Has This Been Tested?

Add some blocks e.g: paragraph, list, heading press the side menu, verify the transformations appear. Apply the transformations and see the transformations happened as expected.
Select a sequence of paragraphs, transform them into a list. Transform the list back into paragraphs.
Add a gallery, transform into images, select all the images and transform into a gallery.

Screenshots (jpeg or gifs if applicable):

screen shot 2017-11-14 at 18 10 39

screen shot 2017-11-14 at 18 11 30

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 13, 2017

Codecov Report

Merging #3458 into master will decrease coverage by 0.22%.
The diff coverage is 58.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3458      +/-   ##
==========================================
- Coverage   37.74%   37.51%   -0.23%     
==========================================
  Files         279      278       -1     
  Lines        6743     6707      -36     
  Branches     1227     1214      -13     
==========================================
- Hits         2545     2516      -29     
- Misses       3536     3539       +3     
+ Partials      662      652      -10
Impacted Files Coverage Δ
editor/components/block-settings-menu/index.js 0% <ø> (ø) ⬆️
editor/components/block-switcher/index.js 4.76% <0%> (+1.9%) ⬆️
...nents/block-settings-menu/block-transformations.js 0% <0%> (ø)
blocks/api/factory.js 88.13% <90.9%> (+1.64%) ⬆️
editor/components/warning/index.js 0% <0%> (-100%) ⬇️
components/panel/color.js 0% <0%> (-100%) ⬇️
components/panel/row.js 0% <0%> (-100%) ⬇️
blocks/autocompleters/index.js 0% <0%> (-42.11%) ⬇️
editor/utils/mobile/index.js 57.14% <0%> (-17.86%) ⬇️
editor/components/post-featured-image/index.js 22.22% <0%> (-17.78%) ⬇️
... and 30 more

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 2fc758d...5a6de55. Read the comment docs.

codecov bot commented Nov 13, 2017

Codecov Report

Merging #3458 into master will decrease coverage by 0.22%.
The diff coverage is 58.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3458      +/-   ##
==========================================
- Coverage   37.74%   37.51%   -0.23%     
==========================================
  Files         279      278       -1     
  Lines        6743     6707      -36     
  Branches     1227     1214      -13     
==========================================
- Hits         2545     2516      -29     
- Misses       3536     3539       +3     
+ Partials      662      652      -10
Impacted Files Coverage Δ
editor/components/block-settings-menu/index.js 0% <ø> (ø) ⬆️
editor/components/block-switcher/index.js 4.76% <0%> (+1.9%) ⬆️
...nents/block-settings-menu/block-transformations.js 0% <0%> (ø)
blocks/api/factory.js 88.13% <90.9%> (+1.64%) ⬆️
editor/components/warning/index.js 0% <0%> (-100%) ⬇️
components/panel/color.js 0% <0%> (-100%) ⬇️
components/panel/row.js 0% <0%> (-100%) ⬇️
blocks/autocompleters/index.js 0% <0%> (-42.11%) ⬇️
editor/utils/mobile/index.js 57.14% <0%> (-17.86%) ⬇️
editor/components/post-featured-image/index.js 22.22% <0%> (-17.78%) ⬇️
... and 30 more

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 2fc758d...5a6de55. Read the comment docs.

@jorgefilipecosta jorgefilipecosta changed the title from Update/added block transforms to block side menu to Added block transforms to block side menu Nov 13, 2017

@jorgefilipecosta jorgefilipecosta changed the title from Added block transforms to block side menu to Show transformations in the ellipsis menu Nov 13, 2017

@jasmussen jasmussen requested a review from karmatosed Nov 13, 2017

@karmatosed

In the mockups there is a separator splitting out the menu items. This visual indicator I think was really useful. Could we add it back in?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Nov 14, 2017

Contributor

Nice work, thanks for working on this. I think this'll be really nice to have — it feels natural for these to be "properties of a multi selection".

In addition to Tammies suggestion, can you move the transformations below the delete button? That way we group transformations all at the bottom.

Contributor

jasmussen commented Nov 14, 2017

Nice work, thanks for working on this. I think this'll be really nice to have — it feels natural for these to be "properties of a multi selection".

In addition to Tammies suggestion, can you move the transformations below the delete button? That way we group transformations all at the bottom.

* @param {Array} blocks Blocks array
* @return {Array} Array of possible block transformations
*/
export function getPossibleBlockTransformations( blocks ) {

This comment has been minimized.

@gziolo

gziolo Nov 14, 2017

Member

Since it is nicely extracted into its own utility method, it would be even better to cover this functionality with unit tests :)

@gziolo

gziolo Nov 14, 2017

Member

Since it is nicely extracted into its own utility method, it would be even better to cover this functionality with unit tests :)

This comment has been minimized.

@aduth

aduth Nov 17, 2017

Member

Since it is nicely extracted into its own utility method, it would be even better to cover this functionality with unit tests :)

Agree with this. Can we get some tests for this?

@aduth

aduth Nov 17, 2017

Member

Since it is nicely extracted into its own utility method, it would be even better to cover this functionality with unit tests :)

Agree with this. Can we get some tests for this?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 27, 2017

Member

tests were added :)

@jorgefilipecosta

jorgefilipecosta Nov 27, 2017

Member

tests were added :)

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 14, 2017

Member

I think the idea here is to remove transformers from the top toolbar:
screen shot 2017-11-14 at 09 37 28

@jasmussen, can you confirm?

Member

gziolo commented Nov 14, 2017

I think the idea here is to remove transformers from the top toolbar:
screen shot 2017-11-14 at 09 37 28

@jasmussen, can you confirm?

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 14, 2017

Member

I like this change a lot, nice work so far 💯

Member

gziolo commented Nov 14, 2017

I like this change a lot, nice work so far 💯

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Nov 14, 2017

Contributor

I think the idea here is to remove transformed from the top toolbar:

Maybe. It depends a little bit.

Right now we default to a top-docked toolbar. The benefit of this is that it's always there in the same spot, never covering the content. The downside is that in some cases, notably around non-text blocks, it can feel detached from the content, compared to the block-docked toolbar, which you can switch to in the ellipsis menu.

We mean to test these two on a bunch of people, see which works the best for the most people. This might define which version we default to, though which ever side we land on it'd be nice to keep the switch around.

The thing is, in the case of the top-docked toolbar, we can do something like this:

direct transformations

That is, have direct transformations linked right in the top toolbar, making it not only contextual to the block selected but indeed the blocks.

We can't really do that with block-docked toolbars, we've explored that in the past and due to the nature of how position: sticky works, we'd have to wrap the selected blocks in a new div on the fly, in order for the toolbar to be able to scroll with you down the page. Complex, slightly hacky, and has its own sets of problems.

So, long story short — there are opportunities in that top area for having direct transformations there. But it depends a little bit on how things play out in tests.

Did that make sense?

Contributor

jasmussen commented Nov 14, 2017

I think the idea here is to remove transformed from the top toolbar:

Maybe. It depends a little bit.

Right now we default to a top-docked toolbar. The benefit of this is that it's always there in the same spot, never covering the content. The downside is that in some cases, notably around non-text blocks, it can feel detached from the content, compared to the block-docked toolbar, which you can switch to in the ellipsis menu.

We mean to test these two on a bunch of people, see which works the best for the most people. This might define which version we default to, though which ever side we land on it'd be nice to keep the switch around.

The thing is, in the case of the top-docked toolbar, we can do something like this:

direct transformations

That is, have direct transformations linked right in the top toolbar, making it not only contextual to the block selected but indeed the blocks.

We can't really do that with block-docked toolbars, we've explored that in the past and due to the nature of how position: sticky works, we'd have to wrap the selected blocks in a new div on the fly, in order for the toolbar to be able to scroll with you down the page. Complex, slightly hacky, and has its own sets of problems.

So, long story short — there are opportunities in that top area for having direct transformations there. But it depends a little bit on how things play out in tests.

Did that make sense?

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 14, 2017

Member

That is, have direct transformations linked right in the top toolbar, making it not only contextual to the block selected but indeed the blocks.

This might be a very interesting pattern when multiple blocks are selected. It's also an improved version of what we have at the moment:

screen shot 2017-11-14 at 10 25 21

So, long story short — there are opportunities in that top area for having direct transformations there. But it depends a little bit on how things play out in tests.

I was a little bit vague. My point was, that there is some duplication with the current implementation which this PR proposes. I think we can keep both versions for the time being and decide later if that is helpful. Code-wise it will be a very straightforward task to adjust it later 👍

The second place where the same action can be triggered:
screen shot 2017-11-14 at 10 27 11

Member

gziolo commented Nov 14, 2017

That is, have direct transformations linked right in the top toolbar, making it not only contextual to the block selected but indeed the blocks.

This might be a very interesting pattern when multiple blocks are selected. It's also an improved version of what we have at the moment:

screen shot 2017-11-14 at 10 25 21

So, long story short — there are opportunities in that top area for having direct transformations there. But it depends a little bit on how things play out in tests.

I was a little bit vague. My point was, that there is some duplication with the current implementation which this PR proposes. I think we can keep both versions for the time being and decide later if that is helpful. Code-wise it will be a very straightforward task to adjust it later 👍

The second place where the same action can be triggered:
screen shot 2017-11-14 at 10 27 11

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Nov 14, 2017

Contributor

My point was, that there is some duplication with the current implementation which this PR proposes. I think we can keep both versions for the time being and decide later if that is helpful. Code-wise it will be a very straightforward task to adjust it later 👍

Right, I have no strong opinion on having both for the time being, or removing the top one. Just that depending on how the top toolbar performs, we might want to revisit that.

Contributor

jasmussen commented Nov 14, 2017

My point was, that there is some duplication with the current implementation which this PR proposes. I think we can keep both versions for the time being and decide later if that is helpful. Code-wise it will be a very straightforward task to adjust it later 👍

Right, I have no strong opinion on having both for the time being, or removing the top one. Just that depending on how the top toolbar performs, we might want to revisit that.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 14, 2017

Member

Hi @karmatosed, thank you for your review, some changes were applied to get closer to the matchups (border added, transformations moved to be after delete), feel free to have a new look (screenshots were updated).

Hi @gziolo, @jasmussen, regarding the duplication of the transforms. Removing them from top bar is very simple and can be easily done. If you prefer I can also create a parallel pull request that shows the direct transformations.

Member

jorgefilipecosta commented Nov 14, 2017

Hi @karmatosed, thank you for your review, some changes were applied to get closer to the matchups (border added, transformations moved to be after delete), feel free to have a new look (screenshots were updated).

Hi @gziolo, @jasmussen, regarding the duplication of the transforms. Removing them from top bar is very simple and can be easily done. If you prefer I can also create a parallel pull request that shows the direct transformations.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 15, 2017

Member

Hi @gziolo, @jasmussen, regarding the duplication of the transforms. Removing them from top bar is very simple and can be easily done. If you prefer I can also create a parallel pull request that shows the direct transformations.

I would trust Joen and Tammie in this regard :) I'm fine with all options, just wanted to make it clear that we have the same options available in two places.

Member

gziolo commented Nov 15, 2017

Hi @gziolo, @jasmussen, regarding the duplication of the transforms. Removing them from top bar is very simple and can be easily done. If you prefer I can also create a parallel pull request that shows the direct transformations.

I would trust Joen and Tammie in this regard :) I'm fine with all options, just wanted to make it clear that we have the same options available in two places.

Show outdated Hide outdated blocks/api/factory.js Outdated
Show outdated Hide outdated blocks/api/factory.js Outdated
Show outdated Hide outdated blocks/api/factory.js Outdated
@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 16, 2017

Member

Hi @aduth, thank you for your review. In fact, I end up applying a huge refactor to getPossibleBlockTransformations (function extracted from block-switcher). I replace all reduce+concat occurrences with flatMap, compact+map, or filter+map. I think this makes the code easier to understand and more performant. Feel free to have a new look.

Member

jorgefilipecosta commented Nov 16, 2017

Hi @aduth, thank you for your review. In fact, I end up applying a huge refactor to getPossibleBlockTransformations (function extracted from block-switcher). I replace all reduce+concat occurrences with flatMap, compact+map, or filter+map. I think this makes the code easier to understand and more performant. Feel free to have a new look.

Show outdated Hide outdated blocks/api/factory.js Outdated
* @param {Array} blocks Blocks array
* @return {Array} Array of possible block transformations
*/
export function getPossibleBlockTransformations( blocks ) {

This comment has been minimized.

@aduth

aduth Nov 17, 2017

Member

Since it is nicely extracted into its own utility method, it would be even better to cover this functionality with unit tests :)

Agree with this. Can we get some tests for this?

@aduth

aduth Nov 17, 2017

Member

Since it is nicely extracted into its own utility method, it would be even better to cover this functionality with unit tests :)

Agree with this. Can we get some tests for this?

Show outdated Hide outdated blocks/api/factory.js Outdated
@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 20, 2017

Member

Hi @aduth, @gziolo, this PR was updated with test cases added to the extracted function. @aduth, thank you for providing deep inputs about how to make the function easier to understand I used most of them.

Member

jorgefilipecosta commented Nov 20, 2017

Hi @aduth, @gziolo, this PR was updated with test cases added to the extracted function. @aduth, thank you for providing deep inputs about how to make the function easier to understand I used most of them.

@aduth

aduth approved these changes Nov 21, 2017

Seems to work nicely, and splitting out the transformation logic seems to lend some more clarity. Still might do for a few inline comments on getPossibleBlockTransformations, but generally 👍

Show outdated Hide outdated editor/components/block-settings-menu/block-transformations.js Outdated
Show outdated Hide outdated editor/components/block-settings-menu/block-transformations.js Outdated
Show outdated Hide outdated editor/components/block-settings-menu/block-transformations.js Outdated
Show outdated Hide outdated editor/components/block-settings-menu/block-transformations.js Outdated
return null;
}
const sourceBlockName = blocks[ 0 ].name;
const blockType = getBlockType( sourceBlockName );

This comment has been minimized.

@aduth

aduth Nov 21, 2017

Member

Kinda makes me wish that <BlockIcon /> could render via the name of the block, so we wouldn't have to get the full type ahead of type in usage.

@aduth

aduth Nov 21, 2017

Member

Kinda makes me wish that <BlockIcon /> could render via the name of the block, so we wouldn't have to get the full type ahead of type in usage.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 27, 2017

Member

Yes, in most cases we have the block, so it makes sense to receive icon but it would make sense to also be possible to pass the name of the block. I will propose this change in a separate PR.

@jorgefilipecosta

jorgefilipecosta Nov 27, 2017

Member

Yes, in most cases we have the block, so it makes sense to receive icon but it would make sense to also be possible to pass the name of the block. I will propose this change in a separate PR.

Show outdated Hide outdated blocks/api/factory.js Outdated
@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 27, 2017

Member

Hi @karmatosed, could you check if the design changes here are ready to ship :)?

Member

jorgefilipecosta commented Nov 27, 2017

Hi @karmatosed, could you check if the design changes here are ready to ship :)?

@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Dec 14, 2017

Member

[edit] Looks like my setup wasn't loading correctly and I now see the design changes. Apologies @jorgefilipecosta on that one. 👍 on the changes.

Member

karmatosed commented Dec 14, 2017

[edit] Looks like my setup wasn't loading correctly and I now see the design changes. Apologies @jorgefilipecosta on that one. 👍 on the changes.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 14, 2017

Contributor

This is looking good to me!

One thing, though — it looks like the separator that separates Settings/Delete from transformations is black. Can you make that one $light-gray-500?

screen shot 2017-12-14 at 14 31 08

Contributor

jasmussen commented Dec 14, 2017

This is looking good to me!

One thing, though — it looks like the separator that separates Settings/Delete from transformations is black. Can you make that one $light-gray-500?

screen shot 2017-12-14 at 14 31 08

jorgefilipecosta added some commits Nov 13, 2017

Extracted getPossibleBlockTransformations from BlockSwitcher componen…
…t to a generic function, improving performance and code readability. Added test cases to the new function.
@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Dec 14, 2017

Member

Hi @jasmussen, nice catch, the change was applied.

Member

jorgefilipecosta commented Dec 14, 2017

Hi @jasmussen, nice catch, the change was applied.

@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Dec 14, 2017

Member

All looks good to me now.. lets ship this! Thanks for working through.

Member

karmatosed commented Dec 14, 2017

All looks good to me now.. lets ship this! Thanks for working through.

@karmatosed karmatosed merged commit e859b0f into master Dec 14, 2017

3 checks passed

codecov/project 38.38% (+0.16%) compared to 29b757d
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/added-block-transforms-to-block-side-menu branch Dec 15, 2017

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