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

Add Align support to Separator block #25147

Merged
merged 4 commits into from
Oct 8, 2020

Conversation

jordesign
Copy link
Contributor

Description

Added support for Align controls (limited to Wide and Full) to Separator block
Addresses #25080

How has this been tested?

Tested in Local (by Flywheel) on Mac OSX in Chrome/Safari.

Tested with TwentyTwenty theme which renders elements as expected (with good theme support for wide end full alignment).

Screenshots

Screen Shot 2020-09-08 at 9 20 13 pm

Types of changes

Change to existing core block - adding additional functionality/support

Checklist:

  • My code is tested.
  • [ x] My code follows the WordPress code style.
  • [x ] My code follows the accessibility standards.

Add alignment support for Separator Block
Align 'wide' and 'full' support added to Separator block
@ItsJonQ
Copy link

ItsJonQ commented Oct 6, 2020

@jordesign Haii! Thanks for working on this one :)

I took it for a spin locally. I think we may need center align there as well.

My thinking is it can serve as the "default". Just in case you update to wide or full, and you want to change it back to the original:

Screen Shot 2020-10-06 at 1 01 25 PM

I don't think we need both align: true and align: [...] 🤔

Hope this was helpful!

@jordesign
Copy link
Contributor Author

Thanks @ItsJonQ - I'm amended that and have an update to the PR

Copy link

@ItsJonQ ItsJonQ left a comment

Choose a reason for hiding this comment

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

🚀 Thanks @jordesign ! With that change, it looks good to me :)

@ItsJonQ
Copy link

ItsJonQ commented Oct 7, 2020

@jordesign Hmm! Before I merge this. I had a question!

Do we need the html: false setting for any reason?

I'm not too familiar with that property. Briefly poking at the other blocks (via code search), I couldn't find many instances of html: false. Just wanted to double check

Thanks!

Copy link

@ItsJonQ ItsJonQ left a comment

Choose a reason for hiding this comment

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

@jordesign Haii! Just checked. I think we need to remove "html": false

@jordesign
Copy link
Contributor Author

Thanks @ItsJonQ - updated now

Copy link

@ItsJonQ ItsJonQ left a comment

Choose a reason for hiding this comment

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

@jordesign Great! Thank you for the speedy adjustments 🙏

@ItsJonQ
Copy link

ItsJonQ commented Oct 8, 2020

The E2E tests are failing... However, I believe we're having E2E testing issues all around.
I'm 99.9999% certain the failures are unrelated to this PR update.

Will proceed with merging.

Thanks again @jordesign !

@ItsJonQ ItsJonQ merged commit a8b8724 into WordPress:master Oct 8, 2020
@ZebulanStanphill
Copy link
Member

@ItsJonQ To reset to the default, you click the currently-selected option. Arguably, there should be an explicit "unset"/"default" option in these kind of dropdown menus, but that's something that should be addressed for all instances of this kind of menu.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Separator Affects the Separator Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants