Added button (call to action) to cover image using nesting #5232
Conversation
jorgefilipecosta
added
[Type] Enhancement
jorgefilipecosta
force-pushed
the
update/add-button-cover-image-nesting
branch
4 times, most recently
from
bbb14d2 to
647eb0d
Compare
jorgefilipecosta
changed the title
| jest.mock( | ||
| '@wordpress/data', | ||
| () => ( { | ||
| withSelect() { |
There was a problem hiding this comment.
This might become hard to maintain. I hope we will find a better way to mock all those HOCs that would work in other cases, too. Latest posts block is another example, where we might have a similar issue.
There was a problem hiding this comment.
yes, I totally agree this may become a problem. I think an option would be for the very used HOCs like withSelect to create a testing util that allows simple predefined mocks, to their behavior.
There was a problem hiding this comment.
Yeah, it makes sense what you said. We need to observe it and performed some refactoring as soon as it becomes hard to maintain 👍
|
Not necessary for this PR, but realized that we aren't limiting the amount of characters you can input. Should we?
When I enable the Call To Action toggle, the button that gets inserted is very wide when empty:
Possibly something with flex and the faux placeholder state we add. I'm seeing an odd issue with the alignments toolbar getting duplicated if I click first the cover image, then the button, then the cover image again:
On a meta level, this seems like the right approach for how to use nesting inside the cover image block. Like the idea of limiting the amount of characters, limiting what types of blocks you can insert inside the cover image makes sense too. |
jorgefilipecosta
changed the title
jorgefilipecosta
force-pushed
the
update/add-button-cover-image-nesting
branch
from
647eb0d to
6c3c353
Compare
|
Thank you for your review @jasmussen.
This problem was fixed, sorry I've missed the commit of one stashed CSS rule that made this behaviour correct.
Nice finding, this seems like an existing problem that affects all nesting. To replicate it in the columns we can add a column, at the start it was no items in the toolbar, then we can add an image inside the column and focus it. We see the toolbar of the image, then if we focus the column in the column toolbar we see all the items of the image toolbar: |
If it breaks the design, yes we should. This is a highly popular visual request and awesome to see it being implemented. My comments align with @jasmussen's. I do think we should test this across devices though when a little more polished. |
Even before the addition of the button, we could break the cover image design if the heading text we added was huge, unfortunately, we can not solve it by limiting the number of characters. |
Probably best we ticket this separatel and explore solutions, so we don't put the whole burden on this PR. But we could look at making it so you can't use linebreaks. Further improvements could be exploring a character limit. GoDaddy's site builder does this:
|
| const toggleParallax = () => setAttributes( { hasParallax: ! hasParallax } ); | ||
| const setDimRatio = ( ratio ) => setAttributes( { dimRatio: ratio } ); | ||
| edit: withSelect( ( select, ownProps ) => ( { | ||
| hasCallAction: !! select( 'core/editor' ).getBlocks( ownProps.id ).length, |
There was a problem hiding this comment.
getBlocks can be an expensive operation since it requires transforming the blocks into its object form, including inner blocks and meta attributes. In this case getBlockOrder would be much cheaper, or even getBlockCount if all you're trying to do is see if there exists an inner block.
|
Wow! Been waiting for this. Any chance you can make it Title, description and buttons? Thanks! |
|
@jorgefilipecosta awesome news :) Thanks! |
Description
This PR adds a toggle that allows to easily nest a button inside the cover image block.
This PR also introduces the concept of "managedNesting", when a block declares support for this feature the block is responsible to have to UI to manage nestings inside it (in the cover image that is done using a toggle). So in nested the blocks, we don't allow insert /block appender, and the remove of nested blocks. The block is responsible for having the UI to do that.
The CSS of the cover image had to refactored, we now use flex-direction column and this changed the way we align elements, there including existing ones.
I'm totally open to discussing alternatives approach to this nesting concept if other ideas appear we can change the managedNesting concept.
How Has This Been Tested?
Add a cover image. Use the text alignment verify it still works as expected ( the CSS had to be refactored).
Use the "Call to action" toggle to add a button verify you can use the button block without problems including the alignment/color options. Save the post and see it looks as expected.
Open a post saved with a button inside a cover image and verify it works as expected.
Closes: #2765
Screenshots (jpeg or gifs if applicable):