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

[FIX] inserter point in mobile app #20195

Merged
merged 3 commits into from
Feb 18, 2020
Merged

Conversation

dratwas
Copy link
Contributor

@dratwas dratwas commented Feb 12, 2020

Description

Gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#1894
I found an issue with the inserter point in the mobile app. The inserter was rendered even if the Add block bottom sheet was hidden. Look at the screen:
Simulator Screen Shot - iPhone X - 2020-02-12 at 12 28 47

It happens because we removed the extraData prop from FlatList. Because of that if the data is the same (it is the same in the case when we open bottom sheet and hide it- we don't change anything ) the renderItem is not called and we end with the wrong state of list items (they are not re-rendered after change of props).
I also fixed the inserter for the first block in an empty post. The Inserter should be rendered instead of a paragraph placeholder.

How has this been tested?

  • Remove all from initial html
  • Click Add Block button
  • The inserter should be shown instead of paragraph placeholder - below the title
  • Add group block
  • add a block in that group
  • the inserter shouldn't be visible after that action and should be in the right place when the bottom sheet is open (please swipe bottom sheet a bit to see if inserter is in the right place)
  • Click and hold on the Add Button (long press) and chose add before or add after and check if inserter is in the right place

Screenshots

Types of changes

Fix for inserter

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.

@dratwas dratwas changed the title fix inserter point in butenberg-mobile [FIX] inserter point in mobile app Feb 12, 2020
@jbinda
Copy link
Contributor

jbinda commented Feb 13, 2020

It also covers below test scenarios from other PR connected with Appender/Separator:

  1. Show Separator in empty post link
  2. Prevent show Separator and AppenderButton in the same time or duplicated Separator line link. See Screenshot section
  3. There is no situation where you cannot open Inserter pressing on Group AppenderButton link. See Screenshot section

With this one there should be no more regression in changes implemented so far.

@dratwas dratwas requested review from hypest and Tug February 14, 2020 15:49
@dratwas
Copy link
Contributor Author

dratwas commented Feb 14, 2020

Hey @hypest, @Tug could you please take a look at it?

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.

LGTM 👍

@@ -116,6 +118,10 @@ export class BlockList extends Component {
scrollViewStyle={ { flex: isRootList ? 1 : 0 } }
data={ blockClientIds }
keyExtractor={ identity }
extraData={
shouldShowInsertionPointBefore ||
Copy link
Contributor

Choose a reason for hiding this comment

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

we could define a forceRefresh variable that has this value and use it here, it would make this change more explicit

Copy link
Contributor

Choose a reason for hiding this comment

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

This would increase code readability

Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

Tested with the steps provided on WPiOS, this is fixing the issue

@dratwas dratwas merged commit fcdd316 into master Feb 18, 2020
@dratwas dratwas deleted the fix/rnmobile/inserter-point branch February 18, 2020 16:30
@github-actions github-actions bot added this to the Gutenberg 7.6 milestone Feb 18, 2020
mchowning pushed a commit that referenced this pull request Feb 18, 2020
* fix inserter point in gutenberg-mobile
mchowning added a commit that referenced this pull request Feb 22, 2020
* Add view with accesibility label on top of floating toolbar (#20267)

* [FIX] inserter point in mobile app (#20195)

* fix inserter point in gutenberg-mobile

* Make sure that all strong tags are removed from title. (#20291)

Co-authored-by: Drapich Piotr <[email protected]>
Co-authored-by: Sérgio Estêvão <[email protected]>
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