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

Dismiss notice overlap fix #11823

Closed
wants to merge 1 commit into from
Closed

Dismiss notice overlap fix #11823

wants to merge 1 commit into from

Conversation

ianbelanger79
Copy link
Contributor

Better targets the dismissable notices to add more padding to the right side to avoid overlaping of the text.

Fixes #11156

Copy link
Contributor

@desrosj desrosj left a comment

Choose a reason for hiding this comment

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

I was able to replicate the issue described in #11156. Tested this branch and it fixed the issue for me! Changes look good.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

components is a generic package, we should avoid using classNames inserted in other packages that are not dependencies of the current pages. For instance, this component can be used in pages outside the Gutenberg page.

padding-right: 36px;
position: relative;
}
.edit-post-layout {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this className needed here? Ideally if the fix here is generic, we can do it without this className, if it's specific to the notices used in the editor, we should override the default styles in the edit-post package instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That className is needed because the original class of .components-notice.is-dismissible, which adds the right padding, gets overridden by this .edit-post-layout .components-notice-list .components-notice. So the idea was to more specifically target the dismissable notices with .edit-post-layout .components-notice-list .components-notice.is-dismissible. You can see the issue here #11156. I'm all for putting this in another place if that makes more sense, I just wasn't sure where it should go.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's move it to where the overriding ( .edit-post-layout .components-notice-list .components-notice ) happens :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Feb 4, 2019
@gziolo
Copy link
Member

gziolo commented Feb 4, 2019

@ianbelanger79 do you plan to address the feedback shared by @youknowriad?

@gziolo gziolo added the [Type] Bug An existing feature does not function as intended label Feb 4, 2019
@gziolo
Copy link
Member

gziolo commented Feb 4, 2019

After some more in-depth check, I figured out that this PR was recently fixed by @Naerriel in #13371. I'm closing this one as a duplicate. @ianbelanger79 thank you for the time spent on this code change.

@gziolo gziolo closed this Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants