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

Fix alignment of notice lists in block placeholders #20024

Merged
merged 1 commit into from
Feb 5, 2020
Merged

Fix alignment of notice lists in block placeholders #20024

merged 1 commit into from
Feb 5, 2020

Conversation

p-jackson
Copy link
Member

@p-jackson p-jackson commented Feb 4, 2020

Description

This change updates the margins of the notice UI added to block placeholders using the withNotices mixin. The update was necessary after #18745 which made the content of block placeholders left-aligned.

Fixes #19671

How has this been tested?

  • Get the File block into error state by uploading an invalid file and then look at the rendered output to observe any layout issues.
  • Tested with File block in column layout
  • Tested mobile width
  • Safari, FF and Chrome on Mac
  • Viewed the fix in a testing block with no other content but the placeholder. This was to check that the fix worked in general, not just for the File block.

Screenshots

Before #18745
Block placeholder content is centred.
before

After #18745
With alignment bug as reported by #19671
image

After this fix
Vertical margins the same as before #18745, but notice is now full width
image

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

This change updates the margins of the notice UI added to block
placeholders using the `withNotices` mixin. The update was necessary
after #18745 which made the content of block placeholders left-aligned.
Copy link
Contributor

@epiqueras epiqueras left a comment

Choose a reason for hiding this comment

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

I don't think we should remove the set width.

@p-jackson
Copy link
Member Author

Thanks for taking a look at this @epiqueras :)

I don't think we should remove the set width.

To my eye at least the full width notice looks a little nicer than having the extra 40px on the right:

image

It's so close to full width that IMO it really feels like it should be anchored to the right-hand side.

I'm happy to add the width rule back if that's the layout we want though. I may have missed something that makes the extra space necessary.

@epiqueras
Copy link
Contributor

@jasmussen Did you add the width there for a reason?

@jasmussen
Copy link
Contributor

I'm sure there was a reason, but that reason isn't there anymore. It seems like both of the margin and width rules can be removed:

widt

@epiqueras epiqueras merged commit da04d71 into WordPress:master Feb 5, 2020
@epiqueras epiqueras added this to the Gutenberg 7.4 milestone Feb 5, 2020
epiqueras pushed a commit that referenced this pull request Feb 5, 2020
This change updates the margins of the notice UI added to block
placeholders using the `withNotices` mixin. The update was necessary
after #18745 which made the content of block placeholders left-aligned.
@p-jackson p-jackson deleted the fix/placeholder-notice-padding branch February 6, 2020 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block Placeholder: Notices
3 participants