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

Button Block: fix space insertion #14925

Merged
merged 3 commits into from Apr 12, 2019
Merged

Button Block: fix space insertion #14925

merged 3 commits into from Apr 12, 2019

Conversation

ellatrix
Copy link
Member

Description

Fixes #14914.

How has this been tested?

Screenshots

Types of changes

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.

@ellatrix ellatrix added the [Block] Buttons Affects the Buttons Block label Apr 11, 2019
@ellatrix ellatrix requested a review from jasmussen April 11, 2019 14:26
@ellatrix ellatrix added this to the 5.5 (Gutenberg) milestone Apr 11, 2019
@jasmussen
Copy link
Contributor

I don't want to block this fix if it makes the Button block unusable in Firefox. But this PR as it is, will cause #10565 to regress back again, so if there is at all time, I would like us to first understand how the whitespace rules breaks things in Firefox even though it should be scoped to just the preview (.block-editor-block-preview__content ).

@ellatrix
Copy link
Member Author

These seem to be the built styles:

.wp-block-button__link {
  background-color: #32373c;
  border: none;
  border-radius: 28px;
  box-shadow: none;
  color: inherit;
  cursor: pointer;
  display: inline-block;
  font-size: 18px;
  margin: 0;
  padding: 12px 24px;
  text-align: center;
  text-decoration: none;
  white-space: normal;
  overflow-wrap: break-word; }

@ellatrix
Copy link
Member Author

@jasmussen Made a mistake. It wasn't your commit that broke it, it seems to be a way older issue! :)

@aduth
Copy link
Member

aduth commented Apr 11, 2019

In my testing, I still see the original issue meant to be addressed by #10565, but ... I also see it in master, so it doesn't seem like something which regressed here.

@@ -15,7 +15,7 @@
// breaking spaces in between words. If also prevent Firefox from inserting
// a trailing `br` node to visualise any trailing space, causing the element
// to be saved.
white-space: pre-wrap;
white-space: pre-wrap !important;
Copy link
Member

Choose a reason for hiding this comment

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

Safe to say this is !important because it breaks spectacularly when any other style takes precedence?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I would say that this is the only CSS rule required for RichText to work properly. It might even be better to insert it inline? Not sure.

@@ -27,7 +27,6 @@ $blocks-button__height: 56px;
padding: 12px 24px;
text-align: center;
text-decoration: none;
white-space: normal;
Copy link
Member

Choose a reason for hiding this comment

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

For posterity's sake: I investigated why this was here in the first place. It was introduced in #5662. I verified the change here does not regress #5628.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

@aduth aduth added the [Type] Bug An existing feature does not function as intended label Apr 12, 2019
@jasmussen
Copy link
Contributor

As noted in #14925 (comment), this will regress #10565. Which is not the end of the world, but also worth noting.

@ellatrix
Copy link
Member Author

@jasmussen How is that possible? I'm not touching the styles in .block-editor-block-preview__content?

@ellatrix
Copy link
Member Author

Ah, in the small previews, they're all RichText instances. That's strange. Why is the save function not used to preview the transforms?

@jasmussen
Copy link
Contributor

The !important would override the preview aspects.

I can't answer why the save function isn't used. It sounds like that might be a good solution? Another would be to provide placeholder content in the previews, though I seem to recall us discussing and dismissing that idea in the past?

@ellatrix
Copy link
Member Author

@jasmussen I fixed it but ideally, it should be revised. :)

@jasmussen
Copy link
Contributor

Thank you! 💕

@ellatrix ellatrix merged commit 5777dbb into master Apr 12, 2019
@ellatrix ellatrix deleted the fix/button-space branch April 12, 2019 16:25
mchowning pushed a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019
* Button Block: fix space insertion

* Typo

* Allow rule for preview
aduth pushed a commit that referenced this pull request Apr 16, 2019
* Button Block: fix space insertion

* Typo

* Allow rule for preview
aduth pushed a commit that referenced this pull request Apr 16, 2019
* Button Block: fix space insertion

* Typo

* Allow rule for preview
This was referenced Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Buttons Affects the Buttons Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Button Block: Adding spaces when typing button text does not work
3 participants