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

Feature/lists #704

Merged
merged 42 commits into from
Apr 5, 2019
Merged

Feature/lists #704

merged 42 commits into from
Apr 5, 2019

Conversation

SergioEstevao
Copy link
Contributor

@SergioEstevao SergioEstevao commented Mar 5, 2019

Refs #208
Gutenberg PR WordPress/gutenberg#14249
New Gutenberg PR WordPress/gutenberg#14636

simulator screen shot - iphone xr - 2019-03-05 at 22 29 37

This PR implements the list block by only changing components and not touching the code of the original web block.

This achieved by improving the implementation of the block-edit context in order to send the isSelected and onFocus props to the RichText components.

For the moment it doesn't implement the indent and outdent buttons, because of a bug in Aztec for iOS that doesn't allow split to work properly when lists are indented.

If testing on iOS please run yarn preios to make sure you get the right version of Aztec.

How to test:

  • Start the demo app
  • Check the list display properly and you can interact with them and change type
  • Add new list block to the post
  • Check that the list block works properly.

@SergioEstevao SergioEstevao added this to the Beta milestone Mar 5, 2019
@SergioEstevao SergioEstevao requested review from koke, Tug and hypest March 5, 2019 22:25
Copy link
Contributor

@hotchemi hotchemi left a comment

Choose a reason for hiding this comment

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

Ignore me if it is out of the point 🙇

react-native-aztec/src/AztecView.js Outdated Show resolved Hide resolved
@SergioEstevao
Copy link
Contributor Author

Here is update of the current status of this PR:

  • Insertion of a new list block
  • Display of list block already present in the content
  • Ability to write inside the lists
  • Convert a block list from ordered ol to unordered 'ul' using the toolbar
  • Indent in is working
  • Indent out is not working
  • When splitting the block an extra paragraph block is being inserted on the middle of the split.

All the above are working on iOS, but on Android we have some issues:

  • Changing from bullets to number list is not working on Android, this is done on JS code so it's strange for me why is not working.
  • Aztec android needs to implement the isMultiline prop added to the component. When this boolean parameter is on onEnter prop should only be invoked when an Enter is pressed and the previous line is an empty line.

@koke do you think after the Android issues we should merge this with a feature flag in place?

@hypest do you want to tackle the Android issue?

@hypest
Copy link
Contributor

hypest commented Mar 28, 2019

As far as I can see on Android, adding a new item to the list by tapping Enter at the end of an item is broken. Instead of adding a new bullet, the block is actually split. Haven't debugged this yet but, @SergioEstevao are you referring to that too when you say Aztec android needs to implement the isMultiline prop added to the component. When this boolean parameter is on onEnter prop should only be invoked when an Enter is pressed and the previous line is an empty line.?

@hypest do you want to tackle the Android issue?

Yeap, added it to my plans for this sprint 👍

@SergioEstevao
Copy link
Contributor Author

SergioEstevao commented Mar 28, 2019 via email

@SergioEstevao
Copy link
Contributor Author

@Tug do you mind to test this. If testing on iOS please run yarn preios to make sure you get the right version of Aztec.

@@ -238,6 +239,7 @@ export default compose( [
isLastBlock,
isSelected,
name,
clientId,
Copy link
Contributor

Choose a reason for hiding this comment

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

withSelect only add new props to the component, so it should not be necessary to explicitly add clientId which is already a prop given from the parent component here.

Copy link
Contributor

@Tug Tug left a comment

Choose a reason for hiding this comment

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

Nice work, we now have List ✋🎉🎈!!

Tested on both Android and iOS and it worked smoothly

@koke koke mentioned this pull request Apr 5, 2019
20 tasks
# Conflicts:
#	react-native-aztec/ios/RNTAztecView/RCTAztecView.swift
@hypest hypest mentioned this pull request Apr 5, 2019
5 tasks
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@SergioEstevao SergioEstevao merged commit d3d67ce into develop Apr 5, 2019
@SergioEstevao SergioEstevao deleted the feature/lists branch April 5, 2019 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants