Skip to content

Conversation

@Tug
Copy link
Contributor

@Tug Tug commented Jan 22, 2019

Fix

Gutenberg has <em> defined as the tag for italic formatting. Aztec can handle <em> but converts it to <i> when sending back the html to gutenberg-mobile. This causes inconsistencies in the UI and should be addressed.

Test

  1. Checkout gutenberg-mobile PR Use format-library for the formatting bar gutenberg-mobile#275
  2. Use the following patch to make changes to Aztec on gutenberg-mobile:
curl "https://gist.githubusercontent.com/Tug/50f0a51b7833a94d5f05f4fa7020a289/raw/3317f9ab4dd096cb2a358c44bb5e1085a8a15550/debug-with-aztec.patch" | git apply -v
  1. Run the app and check that italic formatting is selected when the cursor/selection is on italic
    formatting

Review

@hypest

@Tug Tug self-assigned this Jan 22, 2019
@hypest hypest requested a review from daniloercoli February 6, 2019 22:24
@hypest
Copy link
Contributor

hypest commented Feb 6, 2019

Updated the PR, expanded it to make <em> be the default for italic and recused the tests to test for such. Ready for a review.

👋 @daniloercoli , you think you can make a review pass on this one? Thanks!

@daniloercoli
Copy link
Contributor

Nice! With tests updated this PR can be 🚢ed!

Feel free to merge once the conflicts are resolved.

@hypest
Copy link
Contributor

hypest commented Feb 7, 2019

Thanks for the review @daniloercoli !

Let's hold off merging this until the parent PR is also ready to merge wordpress-mobile/gutenberg-mobile#275. This way we can avoid a potential revert in case the parent PR doesn't get merged.

@hypest
Copy link
Contributor

hypest commented Feb 18, 2019

We're giving the "go ahead with merging" for the Gutenberg and gutenberg-mobile side PRs so, merging this.

@hypest hypest merged commit e02ec13 into develop Feb 18, 2019
@hypest hypest deleted the update/italic-tag-em branch February 18, 2019 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants