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

Use format-library in gutenberg-mobile #12249

Merged
merged 67 commits into from
Feb 18, 2019
Merged

Conversation

koke
Copy link
Contributor

@koke koke commented Nov 23, 2018

Fixes wordpress-mobile/gutenberg-mobile#221

This PR reuses the existing logic for the format buttons in @wordpress/format-library instead of our custom solution added in Aztec.

Moreover, it adds a React Native custom modal component to the format-library Link component which handles creating and editing links. It also create a RN version of URLInput.

This is what it looks like (Not as nice as the mockup yet wordpress-mobile/gutenberg-mobile#221 (comment) but I think we can iterate on it):
image

To test

Check out the gutenberg-mobile PR at wordpress-mobile/gutenberg-mobile#275. Try to use the formatting bar on paragraphs and headings.

@koke koke added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Nov 23, 2018
@Tug Tug force-pushed the rnmobile/rich-text-formats branch 2 times, most recently from 5b04b44 to 1747389 Compare November 27, 2018 12:09
@Tug Tug self-assigned this Dec 6, 2018
@Tug Tug changed the base branch from master to mobile December 13, 2018 22:06
@Tug Tug force-pushed the rnmobile/rich-text-formats branch 3 times, most recently from e9e2419 to d877d6e Compare December 14, 2018 11:53
>
<View style={ { ...styles.content, borderColor: 'rgba(0, 0, 0, 0.1)' } }>
<View style={ styles.head }>
<Button
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't reuse Button from @wordpress/components as they currently have hardcoded styles and it's outside of the scope of this PR to refactor it imo.
Let's use Button from react-native in the meantime even though we might not be able to style them as much as we'd like (wrap text)

@Tug
Copy link
Contributor

Tug commented Dec 18, 2018

This is ready for review!
Pinging @daniloercoli @iseulde and @hypest, if one of you has some time to give it a 👀.
Also, any idea why CircleCI isn't running on this branch? Edit: it seems CircleCI doesn't run for branches that are not based on master

@Tug Tug changed the base branch from mobile to master December 19, 2018 10:50
@Tug Tug force-pushed the rnmobile/rich-text-formats branch 3 times, most recently from 91dfbd8 to 8fb02d7 Compare December 19, 2018 16:14
@Tug Tug force-pushed the rnmobile/rich-text-formats branch from f3188d9 to 0c85a37 Compare December 23, 2018 22:49
@etoledom
Copy link
Contributor

@koke @Tug - I'm working on the images setting, and it uses a bottom-sheet very similar to the one you are developing here.

I'm taking as a base the link/modal.native.js and trying to create a generic-reusable bottom-sheet with the header and styles already set, since it seems like we will be using it in quite a few places.

I created a bottom-sheet.js component but I'm not sure where to place it in the project.
Any ideas?

cc @hypest

@koke
Copy link
Contributor Author

koke commented Jan 23, 2019

I thought most of the work was done by react-native-modal. What is different from Modal in BottomSheet?

@Tug
Copy link
Contributor

Tug commented Jan 23, 2019

@etoledom I gave it a thought actually when writing this component and I think we could create a mobile only component at @wordpress/editor/components/bottom-sheet/index.native.js or maybe even @wordpress/editor/components/mobile/bottom-sheet/index.native.js to be extra clear that it's a mobile component.

@etoledom
Copy link
Contributor

I thought most of the work was done by react-native-modal. What is different from Modal in BottomSheet?

@koke - It's true that most of the hard work is done by Modal. The idea of BottomSheet is to share the base style of our own version of that modal, so it is easier to implement on different parts of the app.

@Tug - That sounds good, I'll give it a try 👍

@Tug
Copy link
Contributor

Tug commented Jan 23, 2019

@koke react-native-modal brings us animations, as well as helper as to how to position the modal. It doesn't have anything about the styling (white background, margin, a drag handle...) or the actions buttons (red Remove, blue Done). In short, a lot of what we have here could be refactor in a reusable component.

@hypest hypest removed the request for review from gziolo February 6, 2019 15:52
@gziolo
Copy link
Member

gziolo commented Feb 7, 2019

The changes applied to web look good. At this point, we have a format library covered with basic e2e tests so this would be caught as a regression :)

I will leave the final ✅ to the mobile team as those changes mostly impact Aztec integration.

If you have any specific question regarding RichText and Format APU @iseulde is the best person to ask, I can try to help, too.

@koke
Copy link
Contributor Author

koke commented Feb 15, 2019

I can't approve since I'm the original PR author, but I'd say this looks good enough.
:shipit: and let's create issues for everything we found in the reviews

@hypest
Copy link
Contributor

hypest commented Feb 18, 2019

We've given the "go ahead with merge" for the parent PR over at the gutenberg-mobile repo, capturing the known issues that will be worked on in follow PRs (see here). Will merge this and iterate.

@hypest hypest merged commit 99f31cd into master Feb 18, 2019
@hypest hypest deleted the rnmobile/rich-text-formats branch February 18, 2019 14:28
@ellatrix
Copy link
Member

@koke @hypest This commit removed underline registration. Could you please add it back?

@gziolo
Copy link
Member

gziolo commented Feb 21, 2019

We should also add a simple e2e test which checks whether all format controls are displayed in the UI to prevent it from happing in the future. Actually, there is an open PR which would cover it indirectly #13455. So we might want to land PR from @tjnicolaides first and figure out the next steps.

@ellatrix
Copy link
Member

This one is not added to the UI. It's only a shortcut. It should get an e2e test, yes.

@hypest
Copy link
Contributor

hypest commented Feb 21, 2019

@hypest
Copy link
Contributor

hypest commented Feb 21, 2019

I can open a PR but, unfortunately don't have the bandwidth to properly test it (we're wrangling the release of v1.0 of gutenberg-mobile), is that OK @iseulde ?

@youknowriad youknowriad added this to the 5.2 (Gutenberg) milestone Mar 4, 2019
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Store current selection in RichText state

* Make native RichText use format-library

This is a work in progress. Sometimes it works, sometimes it doesn't, I haven't
figured out the right way to wire the RichText structure into Aztec

* Fix bad merge

* Don't use applyRecord for now

* Missing newlines

* Do not change lastContent on formatChange, let the component rerender and update record. Also minor cleanup

* Make sure the component rerenders when isSelected changes so FormatToolbar is removed

* Make a simple version of Link that displays on mobile

* Rerender on selection change to update record

* Make it update onFormatChange when url changes

* Cleanup changes in Heading and Paragraph

* Remove unnecessary components and styles added while testing

* Improve link editing UI

* Fix adding a new url without a selection

* Fix editing the text part of the url

* Fill modal URL to current value

* Pressing Remove dismisses the popup. Improve styles

* Bring back multilineTag prop

* Remove unused param in shouldComponentUpdate

* Remove withBlockEditContext for now, it will need a better refactor to properly deal with focus from Aztec

* Temporary fix for whitespace in HTML, will need to be addressed later

* Make sure end is always greater than start for the text selection

* Remove unsupported css property

* Disable autoCapitalize and autoCorrect for URLInput

* Fix lint errors in mobile and in rnmobile/rich-text-formats

* Use a template string to generate html for Aztec in RichText

* Preprocess the HTML value before sending it to Aztec, removing whitespaces in the process

* Use custom button instead of native provided by react-native

* Add some padding to our custom button

* Send active formats to RCTAztecView so we can type new words in a format

* Handle multiple formats with format placeholder

* Cleanup applyFormat

* Make applyFormat support applying multiple formats at once

* Keep formatting when onSelectionChange is emitted without changes

* Keep the same order of formats

* Prevent inserting anything other that a formatting element from the link modal

* Make sure wrapping tags returned by aztec are removed so it's not added to formats

* Improve styling of the modal

* Make sure we don't pass undefined values in activeFormats

* Update formatPlaceholder when user decides to unselect a format in an existing formatted text

* Update props.value on format change

* Force update native view onFormatChange by not setting this.lastContent

* Handle inserting a link

* Fix updating format properties

* Expand link selection automatically so we can edit a link without selecting it explicitly

* Force an aztec text refresh on format change so we make sure the active formats are in sync

* Add comment and make the code more consistent for debugging

* Make sure we don't use format ref inside the placeholder formats

* Make sure we don't use format ref inside the placeholder formats

* Do not try to call valueToFormat on format change without selection

* Fix editing link url

* Add the ability to remove a link without selection

* Do not remove trailing whitespace characters

* Unescape spaces coming from Aztec

* Fix code styling issues

* Pick the formats of the first char when in text start

* Fix lint errors

* RichText value may be undefined

* Update lastEventCount on enter and selection changes

* Update ModalLinkUI to avoid the keyboard

* Lint fixes
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Store current selection in RichText state

* Make native RichText use format-library

This is a work in progress. Sometimes it works, sometimes it doesn't, I haven't
figured out the right way to wire the RichText structure into Aztec

* Fix bad merge

* Don't use applyRecord for now

* Missing newlines

* Do not change lastContent on formatChange, let the component rerender and update record. Also minor cleanup

* Make sure the component rerenders when isSelected changes so FormatToolbar is removed

* Make a simple version of Link that displays on mobile

* Rerender on selection change to update record

* Make it update onFormatChange when url changes

* Cleanup changes in Heading and Paragraph

* Remove unnecessary components and styles added while testing

* Improve link editing UI

* Fix adding a new url without a selection

* Fix editing the text part of the url

* Fill modal URL to current value

* Pressing Remove dismisses the popup. Improve styles

* Bring back multilineTag prop

* Remove unused param in shouldComponentUpdate

* Remove withBlockEditContext for now, it will need a better refactor to properly deal with focus from Aztec

* Temporary fix for whitespace in HTML, will need to be addressed later

* Make sure end is always greater than start for the text selection

* Remove unsupported css property

* Disable autoCapitalize and autoCorrect for URLInput

* Fix lint errors in mobile and in rnmobile/rich-text-formats

* Use a template string to generate html for Aztec in RichText

* Preprocess the HTML value before sending it to Aztec, removing whitespaces in the process

* Use custom button instead of native provided by react-native

* Add some padding to our custom button

* Send active formats to RCTAztecView so we can type new words in a format

* Handle multiple formats with format placeholder

* Cleanup applyFormat

* Make applyFormat support applying multiple formats at once

* Keep formatting when onSelectionChange is emitted without changes

* Keep the same order of formats

* Prevent inserting anything other that a formatting element from the link modal

* Make sure wrapping tags returned by aztec are removed so it's not added to formats

* Improve styling of the modal

* Make sure we don't pass undefined values in activeFormats

* Update formatPlaceholder when user decides to unselect a format in an existing formatted text

* Update props.value on format change

* Force update native view onFormatChange by not setting this.lastContent

* Handle inserting a link

* Fix updating format properties

* Expand link selection automatically so we can edit a link without selecting it explicitly

* Force an aztec text refresh on format change so we make sure the active formats are in sync

* Add comment and make the code more consistent for debugging

* Make sure we don't use format ref inside the placeholder formats

* Make sure we don't use format ref inside the placeholder formats

* Do not try to call valueToFormat on format change without selection

* Fix editing link url

* Add the ability to remove a link without selection

* Do not remove trailing whitespace characters

* Unescape spaces coming from Aztec

* Fix code styling issues

* Pick the formats of the first char when in text start

* Fix lint errors

* RichText value may be undefined

* Update lastEventCount on enter and selection changes

* Update ModalLinkUI to avoid the keyboard

* Lint fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants