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

First stab at having Aztec in the RN app #60

Merged
merged 15 commits into from Jul 2, 2018

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Jun 27, 2018

This PR add an Aztec instance running on the list.

Features:

  1. Using the react-native-aztec repo as a submodule. Local changes to the Aztec component can quickly be tested on the main app.
  2. Looks like a "GB block" but it's not yet. The Aztec component is loaded in a hardcoded way onto the list. Still, the block controls (arrows) work as normal.
  3. Connected with the Redux store. Changes in the text content are mirrored in the store.
  4. The example content loaded in Aztec includes <b> styling to showcase that Aztec is working normally and styles the rendered text.

Gotchas:

  1. This implementation will most probably change in many respects as we work on having a proper GB block (RichText) use Aztec instead of hardcoding Aztec onto the list. But, it's a useful first step as to how to start integrating.
  2. Typing rather fast in the Aztec content leads to some corruption of the content. It's clear that there is some bug in the integration and/or some optimizations in Aztec need to be made in the future.

To test:
Run the app (Android and iOS too) and play with the Aztec "block". On iOS, the Aztec block is just a blue placeholder for now but, it should at least display as such.

@daniloercoli
Copy link
Contributor

Looks good to me!

@diegoreymendez @mzorz, @SergioEstevao Would some of you take a look at this PR and merge in afterwards?

Copy link
Contributor

@mzorz mzorz left a comment

Choose a reason for hiding this comment

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

Went through this, left some comments

type StateType = {
selected: boolean,
focused: boolean,
aztecheight: number,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make aztecheight -> aztecHeight? I think for readability and following how other variables are named

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed with 1995392.

...this.props.attributes,
content: event.nativeEvent.text,
} );
} }
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been considering this a while and opening here for discussion - what do you think about using the ReactAztecFocusEvent and ReactAztecEndEditingEvent events to control when to update the state only, instead of making each TextChange event reflect on the RN kept state in this case?. I understand this is counterintuitive in terms of the React logic, but it rings as something plausible given the performance issues, and is a "pure JS" solution (as opposed to involving Java and more Js/Java interaction controlling code).
I've made tests here just changing onChange for onEndEditing in line 84 up here and it seems to work pretty well (no more lagging / weird refreshing issues). This, without the code in #62 and wordpress-mobile/react-native-aztec#24 cc @daniloercoli @hypest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👋 @mzorz . I'm not well informed yet on the lifecycle events in RN, and in light of wordpress-mobile/react-native-aztec#24 that fixes the editing consistency issue atm, I'd say let's leave the proposal for onEndEditing for a separate PR. I'm actually sure we will visit that since yeah, it is probably suboptimal to update the Redux state on every key press.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is probably suboptimal to update the Redux state on every key press.

👍 Opened #65

@hypest
Copy link
Contributor Author

hypest commented Jun 29, 2018

Thanks for the review @mzorz ! I addressed all the points on the feedback. Feel free to have another look! 🙇‍♂️

@mzorz
Copy link
Contributor

mzorz commented Jun 29, 2018

Thanks for the review @mzorz ! I addressed all the points on the feedback. Feel free to have another look! 🙇‍♂️

This one is good to go as far as I can tell, as we'll keep working on the basis of it. I'd say feel free to merge, but don't know if @diegoreymendez or @SergioEstevao want to give it a look first just in case?

@mzorz
Copy link
Contributor

mzorz commented Jun 29, 2018

Oh BTW and as per conversations, remember #62 should get merged into this branch first to fix the delay/lag problems @hypest

@diegoreymendez
Copy link
Contributor

@mzorz - I'd say move ahead for now, and we can eventually discuss improvements starting next week.

@mzorz
Copy link
Contributor

mzorz commented Jun 29, 2018

👌 @diegoreymendez, then nothing to add @daniloercoli @hypest except this 👇

remember #62 should get merged into this branch first to fix the delay/lag problems

Update AztecRN wrapper to the latest version and fix typing delays
@hypest
Copy link
Contributor Author

hypest commented Jul 2, 2018

OK, I'll leave it to @daniloercoli (as the assignee) to merge this. In the meantime, I merge #62 which even though it's not a blocker for this PR, it's useful and good-to-go anyway 👍.

@daniloercoli
Copy link
Contributor

Cool! Thanks you all for the review!

@daniloercoli daniloercoli merged commit 3d36028 into master Jul 2, 2018
@daniloercoli daniloercoli deleted the feature/aztec-submodule branch July 2, 2018 08:37
hypest pushed a commit that referenced this pull request Nov 2, 2018
Improve dependency checking for scripts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement Improves a current area of the editor
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants