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 list wrap on second line #4229

Merged
merged 1 commit into from Jan 3, 2018

Conversation

Projects
None yet
4 participants
@Rahmon
Contributor

Rahmon commented Jan 2, 2018

Description

Fix list wrap on second line. Fixes #4210

How Has This Been Tested?

This has been tested with "npm test", "npm run test-e2e" and manually on Chrome and Firefox.

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation. #
@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Jan 3, 2018

Member

Thanks for looking into this @Rahmon. The code looks good to me and this doesn't seem to cause any regressions.

Here's how it looks:

screen shot 2018-01-03 at 12 05 19

cc. @karmatosed

Member

noisysocks commented Jan 3, 2018

Thanks for looking into this @Rahmon. The code looks good to me and this doesn't seem to cause any regressions.

Here's how it looks:

screen shot 2018-01-03 at 12 05 19

cc. @karmatosed

@@ -2,6 +2,5 @@
.blocks-list .blocks-editable__tinymce ul,
.blocks-list .blocks-editable__tinymce ol {
padding-left: 1.3em;
list-style-position: inside;
margin-left: 0;
margin-left: 1.3em;

This comment has been minimized.

@mtias

mtias Jan 3, 2018

Contributor

I believe the "inside" was added for some toolbar issues. cc @gziolo

@mtias

mtias Jan 3, 2018

Contributor

I believe the "inside" was added for some toolbar issues. cc @gziolo

This comment has been minimized.

@mtias

mtias Jan 3, 2018

Contributor

It was added here: 1c53b02

@mtias

mtias Jan 3, 2018

Contributor

It was added here: 1c53b02

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Jan 3, 2018

Member

Hi @Rahmon, thank you for your contribution 👍 The "list-style-position: inside;" was added as a fix to the bugs reported in #3727, #3707. This change does not create exactly the same bug, but affects the bullet point position creating a new undesirable behavior:
image
Can be replicated with the following post content https://gist.github.com/jorgefilipecosta/47b6fbac58b6ce66b8a1ffc2fdf02427. This strange behavior is much harder to replicate than the original bugs and the dimensions need to be really specific. I don't see any reason for this bugs to be happening, it looks like a random browser rendering problem and seems to just happen in chrome.

Member

jorgefilipecosta commented Jan 3, 2018

Hi @Rahmon, thank you for your contribution 👍 The "list-style-position: inside;" was added as a fix to the bugs reported in #3727, #3707. This change does not create exactly the same bug, but affects the bullet point position creating a new undesirable behavior:
image
Can be replicated with the following post content https://gist.github.com/jorgefilipecosta/47b6fbac58b6ce66b8a1ffc2fdf02427. This strange behavior is much harder to replicate than the original bugs and the dimensions need to be really specific. I don't see any reason for this bugs to be happening, it looks like a random browser rendering problem and seems to just happen in chrome.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Jan 3, 2018

Member

I did some debugging and it looks like I found a workaround for the new spacing bug not involving list-style-position. So I think this problem is not a blocker and the changes can be merged and I can create a parallel PR that addresses this spacing bullet position problem.

Member

jorgefilipecosta commented Jan 3, 2018

I did some debugging and it looks like I found a workaround for the new spacing bug not involving list-style-position. So I think this problem is not a blocker and the changes can be merged and I can create a parallel PR that addresses this spacing bullet position problem.

@Rahmon

This comment has been minimized.

Show comment
Hide comment
@Rahmon

Rahmon Jan 3, 2018

Contributor

@jorgefilipecosta I saw that you did the last change in editor.scss file so I verified the bugs reported in #3727 and #3707 before submit this PR.. xD

Contributor

Rahmon commented Jan 3, 2018

@jorgefilipecosta I saw that you did the last change in editor.scss file so I verified the bugs reported in #3727 and #3707 before submit this PR.. xD

@mtias mtias merged commit 38b60d6 into WordPress:master Jan 3, 2018

2 checks passed

codecov/project 39.18% remains the same compared to 365b932
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Rahmon Rahmon deleted the Rahmon:fix/4210 branch Jan 3, 2018

getsource added a commit to getsource/gutenberg that referenced this pull request Jan 3, 2018

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