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

Convert to blocks: preserve alignment #19097

Merged
merged 3 commits into from Dec 12, 2019
Merged

Conversation

@ellatrix
Copy link
Member

ellatrix commented Dec 12, 2019

Description

Fixes #11676. When converting to blocks from legacy content, preserve alignment on paragraphs and headings.

How has this been tested?

Add a classic block, then add a paragraph and align it. Convert to blocks. Alignment should be preserved.

Screenshots

align

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
@ellatrix ellatrix changed the title Raw handler: preserve alignment Convert to blocks: preserve alignment Dec 12, 2019
ellatrix added 2 commits Dec 12, 2019
@ellatrix ellatrix merged commit 330c6df into master Dec 12, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@ellatrix ellatrix deleted the try/raw-handler-preserve-alignment branch Dec 12, 2019
@hypest

This comment has been minimized.

Copy link
Contributor

hypest commented Dec 17, 2019

👋, this PR introduces a regression in the native mobile app (wordpress-mobile/gutenberg-mobile#1688) most probably due to the usage of node.style which is not available in the React Native app.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 17, 2019

@hypest Why is it not available there?

@hypest

This comment has been minimized.

Copy link
Contributor

hypest commented Dec 17, 2019

Why is it not available there?

👋 @ellatrix. The jsdom library currently used in the native mobile side doesn't support/parse the style atrributes for the nodes. If I should speculate, I'd assume it's not supported because styling in RN is not applied via CSS.

We had a similar case before (code here), where we have to guard for null node.style. See related discussion here.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 17, 2019

@hypest I expect it to be used much more in the future. Can it be set to an empty object or something?

@mkevins

This comment has been minimized.

Copy link
Contributor

mkevins commented Dec 17, 2019

Hi @ellatrix 👋
Nice work with preserving alignment in the transforms! Regarding the undefined style property:

Can it be set to an empty object or something?

We could set this to {} in jsdom-jscore-rn, but one concern I have with that approach is that we won't know the future cases in which the behavior may deviate from expectation. Another possibility, in this case, would be something like const { textAlign } = node.style || {}.

Solving it on a case-by-case basis like this is not ideal, but at least allows us to see what impact it might have if we "fake" the property. A better long term solution would be to have an implementation of the style property on our HTMLElements in our mobile DOM implementation. A ticket has been opened for this: wordpress-mobile/gutenberg-mobile#1689

@hypest

This comment has been minimized.

Copy link
Contributor

hypest commented Dec 17, 2019

Thanks for adding your thoughts @mkevins !

I agree that only fixing this for the specific breaking case is not a long term solution but it will at least help us to not turn the crashes into silent errors that will manifest elsewhere (which would be harder to debug). Let's go with the const { textAlign } = node.style || {} solution. I'll open a PR.

@ellatrix ellatrix mentioned this pull request Dec 17, 2019
0 of 6 tasks complete
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 17, 2019

I just opened #19196.

@hypest hypest mentioned this pull request Dec 17, 2019
2 of 2 tasks complete
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.