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

Components package: fix component placeholder styling #9776

Closed
wants to merge 1 commit into from

Conversation

vindl
Copy link
Member

@vindl vindl commented Sep 11, 2018

Description

This is an attempt to resolve the styling issue that's present in Gutenberg integration that rely on @wordpress/components package and its CSS. The component placeholder is rendered incorrectly in those cases, and removing the padding from it fixes the issue, while at the same time preserving the expected look in wp-admin.

How has this been tested?

I've verified that this fixes the issue in the Gutenberg integrations that we are working on: wp-calypso and standalone repository (to rule out the possibility of visual issues caused by Calypso shared styles).

After that I've tested the change in Gutenberg local development environment and wp-admin, and made sure that the placeholder still looks the same.

Screenshots

  • In integrations that use @wordpress/components:
Before After
image-misaligned image-aligned
  • There should be no visual changes in wp-admin context.

Types of changes

Bug fix (non-breaking change which fixes an issue).

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Resolve the styling issue that were present in Gutenberg integrations
that rely on published npm packages. Outside of wp-admin context the
removed CSS line was causing the placeholder to appear misaligned.
@vindl vindl added [Package] Components /packages/components [Type] Bug An existing feature does not function as intended labels Sep 11, 2018
@vindl vindl self-assigned this Sep 11, 2018
@vindl
Copy link
Member Author

vindl commented Sep 11, 2018

/cc @jasmussen I tested this locally with Image and Gallery components, not sure if there are other usages that might behave differently.

@jasmussen
Copy link
Contributor

Can you explain to me more why this fix is necessary? It is causing a regression for thin viewports as well as placeholders in nested contexts. With padding:

screen shot 2018-09-11 at 09 59 21

I know that's not the prettiest thing in the world, let's work on that, but the text isn't edge to edge at least.

Without the padding:

screen shot 2018-09-11 at 09 59 33

Not the worst, but the padding inside the placeholder really helps make it not so tight.

@vindl
Copy link
Member Author

vindl commented Sep 11, 2018

Can you explain to me more why this fix is necessary?

Currently I don't have a better explanation than - when components are imported from the package and rendered on the page, the components placeholder appears misaligned (as shown in the screenshots above).

It is causing a regression for thin viewports as well as placeholders in nested contexts

Thanks for catching that! I'll try to find another way to resolve it that doesn't cause regressions.

@jasmussen
Copy link
Contributor

when components are imported from the package and rendered on the page, the components placeholder appears misaligned (as shown in the screenshots above).

I don't see a difference between the before and after, though.

Could flexbox play a role here? I know there are some IE issues already, perhaps a few tweaks here could benefit not only your use case but IE users as well?

@vindl
Copy link
Member Author

vindl commented Oct 17, 2018

I don't see a difference between the before and after, though.

There was not enough padding between the placeholder background (gray) and the right border, and in the latest package versions it's overflowing past that point:

before

In any case, we'll be handling this with Calypso specific overrides so I'm going to close this PR. More info available in Automattic/wp-calypso#27525 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants