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

Try alternate list item jump fix. #12941

Merged
merged 2 commits into from Feb 1, 2019

Conversation

@jasmussen
Copy link
Contributor

jasmussen commented 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.

alternate fix

@jasmussen jasmussen self-assigned this Dec 17, 2018

@jasmussen jasmussen requested review from afercia and WordPress/gutenberg-core Dec 17, 2018

@afercia
Copy link
Contributor

afercia left a comment

I understand the intent to fix it "globally" across the editor but I'm not sure there should be a margin at all. I'd tend to think it should be up to the theme. Twenty Nineteen, for example, doesn't use a bottom margin on the list items so the preview in the editor wouldn't match the front end. Couple more considerations:

  • the 0.5em value seems a bit opinionated, it also depends on the actual font size and may produce a not rounded computed value
  • not sure why the list elements should have a top margin
@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented Dec 17, 2018

To clarify visually, here's a couple screenshots to illustrate the spacing between list items actually changes:

before:

before

after:

after

Noting that either cases don't match the front-end, where the theme (in this case Twenty Nineteen) may use a different value (or no value at all) for the bottom margin.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 17, 2018

I'm not sure there should be a margin at all.

I agree, but given it inherits the margin from WordPress core, and until we can use Shadow DOM to prevent this bleed, are you familiar with any ways to unset this? I suppose we could use unset, though last I checked it was pretty badly supported. It doesn't look terrible anymore? https://caniuse.com/#feat=css-unset-value

the 0.5em value seems a bit opinionated, it also depends on the actual font size and may produce a not rounded computed value

Well it's "half a line-height", which optically feels appropriate for this. In my opinion, sure.

not sure why the list elements should have a top margin

This rule is only for list item containers (ol/ul) that are nested inside others. Did I pull a monday and do something dumb when I could've done something obvious?

@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented Dec 17, 2018

Hm.. maybe we should really consider to take the adventurous route and fix it in core? Alternatively, what about something like margin-bottom: initial;? Wouldn't solve the fact that themes would need to use some higher specificity to override this default, if they will to use some margin.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 17, 2018

I find the spacing between list items to be pretty dang small when unset like this:

initial

However, I do agree it's a better fix for the problem. So for now that's what I've changed it to.

Wouldn't solve the fact that themes would need to use some higher specificity to override this default, if they will to use some margin.

Until such a time as we can prevent CSS bleed from the admin itself, this would be the case, right? I.e. that is unchanged from master and this PR.

I also know that I was of the opinion that we should fix it upstream. However in taking a very brief stab at this, this morning, it felt to me like this should be done in a more holistic admin-redesigny way.

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Dec 17, 2018

The margin is tighter in the change, but agreed that themes should be responsible for improving that experience.

Where possible (and of course it's not always possible) there are always benefits to editor styles being as agnostic as possible.

@youknowriad youknowriad added this to the 4.9 milestone Jan 4, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Jan 15, 2019

I don't understand why the tests are failing here.

Edit: YAY, a rebase and forcepush fixed this.

jasmussen added some commits Dec 17, 2018

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.

@jasmussen jasmussen force-pushed the try/alternate-lists-jump-fix branch from 0d2d36c to 85b5456 Jan 15, 2019

@gziolo gziolo self-requested a review Jan 30, 2019

@karmatosed karmatosed added this to Tighten Up in Phase 2 Jan 31, 2019

@gziolo

gziolo approved these changes Feb 1, 2019

Copy link
Member

gziolo left a comment

LGTM 👍

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 1, 2019

Yay!

@jasmussen jasmussen merged commit 532ed5f into master Feb 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Phase 2 automation moved this from Tighten Up to Done Feb 1, 2019

@jasmussen jasmussen deleted the try/alternate-lists-jump-fix branch Feb 1, 2019

daniloercoli added a commit that referenced this pull request Feb 1, 2019

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/372-use-RichText-on-Title-block

* 'master' of https://github.com/WordPress/gutenberg:
  Try alternate list item jump fix. (#12941)
  Mobile bottom sheet component (#13612)
  Remove unintentional right-margin on last odd-item. (#12199)
  Introduce left and right float alignment options to latest posts block (#8814)
  Fix Google Docs table paste (#13543)
  Increase bottom padding on gallery image caption (#13623)
  Fix the editor save keyboard shortcut not working in code editor view (#13159)
  Plugin: Deprecate gutenberg_add_admin_body_class (#13572)
  Rnmobile/upload media failed state (#13615)
  Make clickOnMoreMenuItem not dependent on aria labels (#13166)
  Add: className prop support to server side render. (#13568)
  Fix: Categories Block: hierarchical Dropdown (#13567)
  Docs: Add clarification about git workflow (#13534)
  Plugin: Remove `user_can_richedit` filtering (#13608)
  eslint-plugin: Add rule `no-unused-vars-before-return` (#12828)
  Image settings button (#13597)
  Fixed wording for the color picker saturation (#13479)

# Conflicts:
#	packages/block-library/src/image/edit.native.js

@jasmussen jasmussen referenced this pull request Feb 7, 2019

Merged

RichText: Format boundaries without DOM #13697

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment