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

Enhancement: Introduce text alignment setting to subhead block #6525

Merged
merged 3 commits into from
May 10, 2018
Merged

Enhancement: Introduce text alignment setting to subhead block #6525

merged 3 commits into from
May 10, 2018

Conversation

nfmohit
Copy link
Member

@nfmohit nfmohit commented May 1, 2018

Description

This PR addresses #6447 which requested a text alignment option (left/centre/right) in the subhead block as we can see in the heading block. It introduces the "Subhead Settings" panel and the text alignment option inside it for the subhead block.

How has this been tested?

This PR has been tested in the Gutenberg Editor for both posts and pages, specifically in the block settings under edit post sidebar for the "Subhead" block. This was tested in WP 4.9.5, Apache server with PHP 7.2.0 and MySQL 5.6.34. According to initial tests, the code doesn’t seem to affect any other areas.

Screenshots

gutenberg-min

Types of changes

This PR imports the dependencies required for the subhead settings panel and text alignment toolbar ( i.e. Fragment, PanelBody, InspectorControls and AlignmentToolbar ), adds the align constant variables in the edit and save functions and lastly outputs the panel.

Note

Gutenberg Editor in post/pages containing existing subhead blocks before this PR is fetched will malfunction after the fetch. Existing subhead blocks will not be editable showing the following message:

This block appears to have been modified externally.

To overcome this, the existing subhead blocks will need to be deleted before fetching this PR. It is recommended to test the PR in a post/page with no existing subhead blocks.
(Please disregard, this issue was specific to my local setup. See comment.)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@ajitbohra
Copy link
Member

@nfmohit existing blocks can be gracefully handled using Deprecated Blocks https://wordpress.org/gutenberg/handbook/block-api/deprecated-blocks/

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.

To overcome this, the existing subhead blocks will need to be deleted before fetching this PR. It is recommended to test the PR in a post/page with no existing subhead blocks.

It isn't an acceptable solution. It needs to be ensured that all blocks created before aren't affected. The easiest approach would be to never apply style when align is undefined - which is the case for all blocks that don't have this property as of today.

@nfmohit
Copy link
Member Author

nfmohit commented May 2, 2018

Thank you guys!

I completely understand that the PR at this stage couldn't be accepted as a solution as I mentioned that existing subhead blocks were malfunctioning. I was looking into this as more of an in-development PR and was looking forward to suggestions how I could overcome that limitation.

Thank you so much for the idea @ajitbohra, I was completely unaware of the Deprecated Blocks API. It has just been about a week working with Gutenberg (and I love it so far, I believe it has great possibilities), I'm feeling a bit hard to digest everything altogether.

It seems I have kinda reached another dead end here. Now that I re-tested the PR in multiple environments, it seems like the issue cannot be replicated anymore. I believe I had a default parameter set in the align attribute with a value of left during my initial tests, which was enforcing the subhead block to add the style attribute in the paragraph tag even when align was undefined. I got rid of that but didn't test the mentioned limitation without the default, which seems to be working fine now.

With that said, would you be able to test the PR once and see if you can replicate the issue @gziolo (and anyone else)? I really thank you for looking into this at the first place and I apologize for the inconvenience. If you can, please point me out any flaws that you might find so I can work on that. I'm sorry if I'm missing something again. Thank you very much!

@gziolo
Copy link
Member

gziolo commented May 4, 2018

I tested it myself and I don't see any warnings, so I think it must have been your local setup. I didn't check it myself before, I replied based on your comment left in the description :)

I can see all text alignement options.
screen shot 2018-05-04 at 09 48 19

However, they are located in the sidebar which we are trying to avoid, as far as I remember. I hope @karmatosed can confirm that we want to have it inside block's toolbar for consistency.

@gziolo
Copy link
Member

gziolo commented May 4, 2018

However, they are located in the sidebar which we are trying to avoid, as far as I remember. I hope @karmatosed can confirm that we want to have it inside block's toolbar for consistency.

Heading has it located in the sidebar, too. I'm confused. I'm leaving the final 👍 to @karmatosed.

@gziolo gziolo added [Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement. labels May 4, 2018
@gziolo gziolo added this to the 2.9 milestone May 4, 2018
@gziolo gziolo added the Needs Design Feedback Needs general design feedback. label May 4, 2018
@nfmohit
Copy link
Member Author

nfmohit commented May 4, 2018

Thank you so much for reviewing this again @gziolo!

I placed it in the sidebar as #6447 this PR addresses discusses more about heading. Looking forward to @karmatosed's feedback so that I can change the position if required.

@karmatosed
Copy link
Member

I don't think they should be in the sidebar, they should have them, but not there.

@nfmohit
Copy link
Member Author

nfmohit commented May 10, 2018

Thanks @karmatosed ! I've added a new commit which moves the alignment settings from the sidebar to the block toolbar.

Here's a screencast showing the change and workflow:
gutenberg-min

@gziolo
Copy link
Member

gziolo commented May 10, 2018

Yes, this looks good, @nfmohit thanks for moving those controls to the block's toolbar 👍

@gziolo gziolo merged commit 67207b7 into WordPress:master May 10, 2018
@gziolo
Copy link
Member

gziolo commented May 10, 2018

Travis complains about PHP 5.2 targeted build, but everything looks good. Moving on regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants