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

Tidy up button vertical align styles #17601

Merged
merged 2 commits into from
Sep 30, 2019
Merged

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Sep 26, 2019

NOTE - I don't think should be merged for WP5.3 as it affects every Button. Having said that, it looks like buttons maybe updated anyway - #17585.

Description

This is a follow-up to #17572 (comment)

Primary buttons had vertical-align: top, but other types of buttons didn't, which made laying them out alongside one another difficult. This PR removes that vertical-align: top from all buttons to make them consistent. It affects all buttons so it might have some far-reaching consequences (but I couldn't see any particular issues).

I've removed the slightly hacky fixes from #17572 and switched to flexbox's align-items: baseline to properly align the buttons in the block warnings.

There were also some other minor tweaks required

How has this been tested?

Manual testing

  • Test invalidating a block and viewing the warning message (the easiest way is to switch to the code editor and add a self closing div in a block)
  • Testing missing blocks and viewing the warning message (install a block, add it to a post, then deactivate it)
  • Test the editor error boundary message (throw an exception somewhere (like BlockList))
  • Test the block error boundary message (throw an error in a block edit) (oh, turns out block error boundaries are not working!).

Screenshots

Screen Shot 2019-09-26 at 4 46 59 pm
Screen Shot 2019-09-26 at 4 41 17 pm

Types of changes

Task

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.

@talldan talldan added [Type] Task Issues or PRs that have been broken down into an individual action to take [Component] Button labels Sep 26, 2019
@talldan talldan self-assigned this Sep 26, 2019
@youknowriad
Copy link
Contributor

Do you know why primary buttons had that vertical-align style?

@youknowriad
Copy link
Contributor

Capture d’écran 2019-09-26 à 10 01 32 AM

In Twenty Twenty, the warning message is not full width, I'm not sure if that's expected or not.

@talldan
Copy link
Contributor Author

talldan commented Sep 27, 2019

Do you know why primary buttons had that vertical-align style?

@youknowriad It seems to have been there from #6562, so from pretty early on.

It's not clear why it was added, but I imagine it was lifted from the styles in core ... it does look like core buttons have vertical-align: top specified everywhere.

It seems unnecessary with modern css and a little over-opinionated. The fewer styles in these base components the better.

In Twenty Twenty, the warning message is not full width, I'm not sure if that's expected or not.

Just had a look. That only seems to be only happening on the Button block. For some reason the button block has display: table; from #16873, which is causing this issue.

I wonder if a lint rule to stop display: table might be an idea.

@youknowriad
Copy link
Contributor

display: table is there on purpose to allow the block preview to only take the button size into consideration. Why do you think display: table should be avoided?

@talldan
Copy link
Contributor Author

talldan commented Sep 27, 2019

display: table is there on purpose to allow the block preview to only take the button size into consideration. Why do you think display: table should be avoided?

Maybe it's just me, but I feel like display: table; is unpredictable with regards to how it'll affect children, as evidenced by that layout issue above. I don't really have an explanation for why that's happened or a good idea of how to fix it.

If theres a way to apply that rule just for previews, that might be a solution.

@youknowriad
Copy link
Contributor

I've used display: table to make something display as an inline-block by taking the width of its content but also the next elements should show under the element and not after it like a regular inline-block would do.

Can't we just absolute align the warning left: 0 and right: 0 compared to the block wrapper (not the block itself) like the white background we see under the warning basically?

Also, I feel this is something that can be explored separately from this PR.

@talldan
Copy link
Contributor Author

talldan commented Sep 30, 2019

@youknowriad I haven't been able to find a way to fix it yet. I think it also speaks to a different problem—that block styles shouldn't have such an effect on general editor styles.

Will merge this PR as I'm working on updating the button styles now.

@talldan talldan merged commit 96a4153 into master Sep 30, 2019
@talldan talldan deleted the tidy/button-vertical-align branch September 30, 2019 04:13
@talldan talldan added this to the Gutenberg 6.6 milestone Sep 30, 2019
youknowriad pushed a commit that referenced this pull request Sep 30, 2019
* Remove vertical-align:top from buttons and tweak block-editor-warning message

* Styling tweaks for block warnings to account for removed `vertical-align: top`
youknowriad pushed a commit that referenced this pull request Sep 30, 2019
* Remove vertical-align:top from buttons and tweak block-editor-warning message

* Styling tweaks for block warnings to account for removed `vertical-align: top`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants