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 'jump' for nested items in list #12590

Closed

Conversation

Naerriel
Copy link
Contributor

@Naerriel Naerriel commented Dec 4, 2018

Description

Fix 'jumping' of nested items in list: #12526
Change margin-bottom and line-height in block-list file.

How has this been tested?

I have ran 'npm run test' and have seen that all tests passed or skipped.

Screenshots

block-list

Types of changes

Minor bug fix: #12526

Checklist:

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

@ellatrix ellatrix requested a review from jasmussen December 4, 2018 17:37
@jasmussen
Copy link
Contributor

Thanks for this PR!

As I noted in #12526 (comment), I think the real fix is to make us not have to override this style. But perfect should not be the enemy of good and this might be worth merging as such.

But I'd appreciate a sanity check by @chrisvanpatten, as this touches style scoping, and he seems to be skilled in that area ;)

@Naerriel
Copy link
Contributor Author

Naerriel commented Dec 4, 2018

If I understand correctly, the same problem with jumping would occur in every Wordpress list but it wouldn't be as visible as in Gutenberg, because you can't nest in an instant a list element.

Do I get it right?
Do you want me to fix this? 😄
If yes, do you want me to fix this the same way?

@jasmussen
Copy link
Contributor

We do need to fix it, yes, and your fix might be fine. I just want to verify we shouldn't fix it in a different way, so awaiting a response from Chris when he has a minute. (Thanks Chris)

@chrisvanpatten
Copy link
Contributor

I think the only potential change to reduce specificity a bit would be something like this:

.block-library-list .editor-rich-text__tinymce,
.block-library-list .editor-rich-text__tinymce ul,
.block-library-list .editor-rich-text__tinymce ol {
	padding-left: 1.3em;
	margin-left: 1.3em;
}

.block-library-list .editor-rich-text__tinymce li {
	margin-bottom: 0;
	line-height: 2;
}

Admittedly I haven't had time to test this so take that with a grain of salt.

Also, is the line-height necessary? @afercia implied in the original ticket that margin-bottom should be enough. Setting line-height seems like it could have other implications…

@Naerriel
Copy link
Contributor Author

Naerriel commented Dec 5, 2018

It isn't necessary but then the list elements would be much closer together, than before. I can skip it.

@afercia
Copy link
Contributor

afercia commented Dec 5, 2018

I'd tend to think the line-height should be inherited from the theme.

@chrisvanpatten
Copy link
Contributor

Agreed w/ @afercia that we should let themes set the line height on these elements :)

@Naerriel
Copy link
Contributor Author

Naerriel commented Dec 6, 2018

Is that now all right, or should I rather modify core wordpress styles?

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Block] List Affects the List Block Needs Decision Needs a decision to be actionable or relevant labels Dec 14, 2018
@gziolo
Copy link
Member

gziolo commented Dec 14, 2018

Can we decide whether this should be fixed in core or here?

jasmussen pushed a commit that referenced this pull request Dec 17, 2018
This PR is an alternative to #12590, and also fixes #12526. Props @Naerriel for initial work and inspiration.

The different approach taken here is to embrace that we are applying a specific margin to our list items and overrides bleed from wp-admin. In doing so it moves these margins to the editor styles stylesheet, which is a more appropriate place for it.
@jasmussen
Copy link
Contributor

Thank you for your PR!

I took a stab at fixing this in WordPress upstream, but decided that this wasn't the place to fix it. In wp-admin it's, for better or worse, a design decision. The reason it wasn't an issue in the past is the editing canvas was in an iframe, preventing CSS bleed.

In the future, Shadow DOM will hopefully allow us to accomplish something similar here. In the mean time, that means we need to fix it in Gutenberg. But it also means we need to fix it in a way that has us "embrace" it. For that reason I took a different approach and created #12941. Can you take a look and see if that PR can replace this? Thank you for your work.

@Naerriel
Copy link
Contributor Author

Yes, I think that #12941 can replace mine and I prefer your approach as it doesn't change the initial style.

Thank you!

@Naerriel Naerriel closed this Dec 17, 2018
jasmussen pushed a commit that referenced this pull request Jan 15, 2019
This PR is an alternative to #12590, and also fixes #12526. Props @Naerriel for initial work and inspiration.

The different approach taken here is to embrace that we are applying a specific margin to our list items and overrides bleed from wp-admin. In doing so it moves these margins to the editor styles stylesheet, which is a more appropriate place for it.
jasmussen added a commit that referenced this pull request Feb 1, 2019
* Try alternate list item jump fix.

This PR is an alternative to #12590, and also fixes #12526. Props @Naerriel for initial work and inspiration.

The different approach taken here is to embrace that we are applying a specific margin to our list items and overrides bleed from wp-admin. In doing so it moves these margins to the editor styles stylesheet, which is a more appropriate place for it.

* Move to "initial".
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Try alternate list item jump fix.

This PR is an alternative to #12590, and also fixes #12526. Props @Naerriel for initial work and inspiration.

The different approach taken here is to embrace that we are applying a specific margin to our list items and overrides bleed from wp-admin. In doing so it moves these margins to the editor styles stylesheet, which is a more appropriate place for it.

* Move to "initial".
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Try alternate list item jump fix.

This PR is an alternative to #12590, and also fixes #12526. Props @Naerriel for initial work and inspiration.

The different approach taken here is to embrace that we are applying a specific margin to our list items and overrides bleed from wp-admin. In doing so it moves these margins to the editor styles stylesheet, which is a more appropriate place for it.

* Move to "initial".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] List Affects the List Block Needs Decision Needs a decision to be actionable or relevant [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List block: avoid "jump" when nesting a list item
5 participants