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: Button block does not centers on the editor #17063

Merged

Conversation

@jorgefilipecosta
Copy link
Member

commented Aug 16, 2019

Description

Fixes: #17059
Regressed in:#16873

In #16873 we started using a display table for the button block in the editor. As with this display mode, the previews look better. But this introduces a regression as in this mode align-center did not work as expected.
We also relied on withFallbackStyles to directly change the dom and add a class. We should avoid direct dom changes so this PR removes the class addition and uses a more complex CSS selector to avoid the need for that class.
This PR also only applies display table for the previews, as using a display table in normal block operation may be unexpected.

With these changes, the block itself got correctly center aligned but the link UI did not align as expected so a margin for the link UI was added.

How has this been tested?

I added a button block.
I tried all the alignment options in and verified the result was the expected one in the editor and in the styles preview.

@@ -76,6 +80,8 @@
}
}

.wp-block-button-wrapper {
// Use display table for block previews.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 16, 2019

Contributor

Conceptually, I don't think we should be having styles that target only previews.
The reason for the display: "table" is that the button container should only wrap the content and not takes the full width. I wonder if there's a way to keep the display table and fix the centering?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Aug 16, 2019

Author Member

Hi @youknowriad, thank you for the context I submitted an alternative solution that keeps the display table and fixes the centering.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/button-block-doees-not-centers-in-the-editor branch from 66a54d3 to 3557044 Aug 16, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

The preview is working properly right now but there's a small issue with the URL input, it's width adapts to the available width, which means if you have just a small button, the input is "cropped".

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

The preview is working properly right now but there's a small issue with the URL input, it's width adapts to the available width, which means if you have just a small button, the input is "cropped".

Hi @youknowriad, I think this "cropped" behavior will always happen as long as we use the table display mode. The solution I see is not using table display when the block is selected I updated this PR to use this logic.
Other option would be a media query to set a width on the input but it will not work well in nested scenarios for example.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

@jorgefilipecosta why are we using max-width instead of just width (especially for the medium and large breakpoints) for the input? OR maybe do we need min-width for the input at all?

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

@jorgefilipecosta why are we using max-width instead of just width (especially for the medium and large breakpoints) for the input? OR maybe do we need min-width for the input at all?

I'm not deep inside the reason but I guess, max-width allows us to limit the width but if the button is nested in a small area we will be able to have a small input while if we used width the button would become big and may overflow the available space.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

I'm not deep inside the reason but I guess, max-width allows us to limit the width but if the button is nested in a small area we will be able to have a small input while if we used width the button would become big and may overflow the available space.

If that's the reason, cropping the input for small buttons seems fine 🤷‍♂

@jorgefilipecosta jorgefilipecosta merged commit fbf1938 into master Aug 16, 2019
4 checks passed
4 checks passed
Filter opened
Details
Filter opened
Details
Milestone It
Details
Travis CI - Pull Request Build Passed
Details
@jorgefilipecosta jorgefilipecosta deleted the fix/button-block-doees-not-centers-in-the-editor branch Aug 16, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

If that's the reason, cropping the input for small buttons seems fine 🤷‍♂

Yes, I also think so. @jasmussen, @kjellr any thoughts on this? should we refactor these styles?

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

Here's a screenshot for reference:

button-crop

any thoughts on this? should we refactor these styles?

Yeah, I think we should refactor. That cropping isn't totally ideal. I've opened #17086 to keep track of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.