Skip to content
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

Used align hook in columns block; Fixed columns alignment bug; #6364

Merged
merged 1 commit into from
Apr 26, 2018

Conversation

jorgefilipecosta
Copy link
Member

This PR makes column class make use of the align hook, by using supports: align. This change fixes a bug where align classes were not passed to the frontend.

Fixes: #5117

How has this been tested?

Verify the columns block continue to work as expected, verify it is possible to align it to 'wide' or 'full' and then the corresponding class is passed to the frontend.

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Type] Enhancement A suggestion for improvement. labels Apr 23, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Apr 23, 2018
@@ -70,20 +69,11 @@ export const settings = {
},

edit( { attributes, setAttributes, className } ) {
const { align, columns } = attributes;
const { columns } = attributes;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to introduce a similar change in #5099. If you look at https://github.com/WordPress/gutenberg/pull/5099/files#diff-89d3b001ccbb88ada880aa2d5db1c55bR58, getEditWrapperProps was also removed because it is handled by hooks.

@gziolo
Copy link
Member

gziolo commented Apr 24, 2018

I did the same in #5099, we use align with Paragraph block. However, we didn't roll this out to all blocks because we didn't find a solid way to tackle server-side rendered blocks. The issue is that those hooks that modify blocks to add support for align are executed only on the client. There needs to be a way to have the same behavior on the server to make sure that align attributes is listed when rendering dynamic block.

On the technical level, this PR is good to go after no longer needed getEditWrapperProps is removed. The question remains if we should do it or wait as @aduth suggested until this solution is bullet-proof.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/used-align-hook-columns branch 2 times, most recently from fe79e16 to cf38d4b Compare April 26, 2018 13:51
@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Apr 26, 2018

Hi @gziolo, I'm sorry I did not remember that your PR also did this change. Given that your PR is on hold until we solve the server side blocks challenge, I think it would be ok to merge this one as it solves a bug. We are already using supports: align in some blocks like audio so it would basically be one more block using it :)
getEditWrapperProps was removed.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to go as I mentioned earlier.

@jorgefilipecosta jorgefilipecosta changed the title Used align hook in columns block. Used align hook in columns block; Fixed columns alignment bug; Apr 26, 2018
@jorgefilipecosta jorgefilipecosta merged commit 2e868b1 into master Apr 26, 2018
@jorgefilipecosta jorgefilipecosta deleted the fix/used-align-hook-columns branch April 26, 2018 14:20
@jorgefilipecosta jorgefilipecosta added this to the 2.8 milestone Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alignment class missing from column block save function.
2 participants