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 the block preview padding in themes with custom backgrounds #17807

Merged
merged 1 commit into from
Oct 8, 2019

Conversation

youknowriad
Copy link
Contributor

closes #17777

This PR removes the padding around the block preview from the help panel in the inserter and instead adds a padding prop to the BlockPreview component in order to apply that padding inside the div affected by the editor styles.

Testing instructions

  • Use the TwentyTwenty theme
  • Hover the Image block in the inserter
  • Notice that the theme's background color is applied to the padding around the images

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended [Feature] Inserter The main way to insert blocks using the + button in the editing interface labels Oct 7, 2019
@youknowriad youknowriad self-assigned this Oct 7, 2019
@retrofox
Copy link
Contributor

retrofox commented Oct 7, 2019

My immediate thoughts about these changes are why we are not trying to deal with this through CSS properties, even adding a new element for this. It seems to add extra (and maybe unnecessary) complexity to the preview block.
However, I've confident about your criteria and pretty sure you already have enough reasons to suggest this approach.
I just would like to know why, if it's possible.

Copy link
Contributor

@mapk mapk left a comment

Choose a reason for hiding this comment

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

The code does seem a bit convoluted with padding values injected there instead of separated out. But I understand the need. I messed around purely in CSS this weekend and couldn't get the preview to format correctly. Thanks for making this happen, @youknowriad. Looks good to merge.

@youknowriad
Copy link
Contributor Author

I just would like to know why, if it's possible.

Sure, The reason is that we scale the block relatively to the parent container and position it absolutely. That parent container is the container that applies the editor styles. Maybe we could add another intermediary div and rely on padding, but I wasn't sure about potential impacts of another div in the DOM tree there, so I endup computing the "padding" by altering the "left" and "top" values.

@retrofox
Copy link
Contributor

retrofox commented Oct 7, 2019

That parent container is the container that applies the editor styles

Is editor-styles-wrapper the CSS class used to apply these styles?

Thanks for your answer, and please feel free to go ahead. Don't take this as a blocker. 🙇

@youknowriad
Copy link
Contributor Author

Is editor-styles-wrapper the CSS class used to apply these styles?

yes :)

@retrofox
Copy link
Contributor

retrofox commented Oct 8, 2019

Is editor-styles-wrapper the CSS class used to apply these styles?

yes :)

We've had to deal with these kinds of issues previewing blocks. What we did was simply adding the editor-styles-wrapper CSS class on the block preview container. The scale and position aren't applied to this element but its child, avoiding get affected by the previewing process.
The funny thing about that is we are adding the same CSS class to more than one element however it doesn't seem to be a big deal.

Screen Shot 2019-10-07 at 3 29 04 PM

I guess applying the padding to that element should work.

@youknowriad
Copy link
Contributor Author

The funny thing about that is we are adding the same CSS class to more than one element however it doesn't seem to be a big deal.

yes, I can see how this can work in almost all themes but I'd personally prefer just one element to avoid potential theme conflicts.

@youknowriad youknowriad merged commit e244f20 into master Oct 8, 2019
@youknowriad youknowriad deleted the fix/block-preview-spacing branch October 8, 2019 13:58
@retrofox
Copy link
Contributor

retrofox commented Oct 8, 2019

The funny thing about that is we are adding the same CSS class to more than one element however it doesn't seem to be a big deal.

yes, I can see how this can work in almost all themes but I'd personally prefer just one element to avoid potential theme conflicts.

I think it will apply the same styles for each element again and again, where the CSS class is applied. Ok, I don't see how this could generate theme conflicts:

<div className="editor-styles-wrapper">
  <div className="editor-styles-wrapper">
    <div className="editor-styles-wrapper custom-css">
      Hi!
    </div>
  </div>
</div>
.custom-css {
  padding: 2px;
}

@youknowriad
Copy link
Contributor Author

@retrofox yes, thinking of things like multiple paddings, relative units... Granted that the changes are small especially if we reset things (like paddings, margins)... but there's still a chance of breakage because themes won't test against this when working on their editor styles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inserter help panel - change padding styling on block examples
3 participants