Fix for Placeholder that is missing on heading block by daniloercoli · Pull Request #739 · wordpress-mobile/gutenberg-mobile

daniloercoli · 2019-03-12T19:04:50Z

This PR does fix the Placeholder text that is missing on the heading block (and probably other non-paragraph text blocks too).

In this PR we're passing down the blockType prop to the native wrapper, and use it to wrap the text the first time the user inserts a new value in the field. In this way Aztec properly render the text on the screen and the GB toolbar updated accordingly.

~~I still need to re-check the code that does adds the tags around the text, since i'm not fully convinced it's safe, but the general idea is here.~~

GB side PR available here: WordPress/gutenberg#14404

To test this PR

Test 1

Add a new heading block
The placeholder text should appear
Start typing in the block
The styling should be there and the toolbar updated

Test 2

Remove all the text in a Heading block
The placeholder text should appear again
Start typing in the block
The styling should be there and the toolbar updated

Test 3

Add a new heading block
The placeholder text should appear
Paste a text in the block
The styling should be there and the toolbar updated

… it to wrap the text inserted by the user.

…rg-mobile into issue/707-Placeholder-is-missing-on-Heading-block * 'develop' of https://github.com/wordpress-mobile/gutenberg-mobile: Update caret position after inserting link (#716) # Conflicts: # gutenberg

marecar3 · 2019-03-13T18:06:48Z

Nice work @daniloercoli!
Seems that you found a robust solution which is good :)

While I was testing this PR on my Nexus 5x, Android: 8.0, I have noticed some glitch.
When you create a new heading block and after you type in a first letter you can notice that heading input field will resize very slow.

I am wondering, what amount of work can it take to assign same formatting style (<h1>)for the Placeholder like iOS has or at least to increase the initial size of RichText when type is heading, WDYT ?

marecar3 · 2019-03-13T18:52:50Z

...ive-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java

+
+            // Add the outer tags when the field was started empty, and only the first time the user types in it.
+            if (mPreviousText.length() == 0 && currentEventCount == 1 && !TextUtils.isEmpty(mEditText.getTagName())) {
+                mEditText.fromHtml('<' + mEditText.getTagName() + '>' + newText + "</" + mEditText.getTagName() + '>', false);


Is there maybe some Util that we can use to generate the html from tag name and text in order to have one place for adding the <,> when there is a need for them?

Not that i'm aware of, but in this case it's pretty simple the input string, that doesn't worth introduce a new function for it. We can use StringBuilder, but it's a very simple string.

marecar3 · 2019-03-13T18:58:12Z

...ive-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java

                            start + before));
+
+            // Add the outer tags when the field was started empty, and only the first time the user types in it.
+            if (mPreviousText.length() == 0 && currentEventCount == 1 && !TextUtils.isEmpty(mEditText.getTagName())) {


If you agree we can probably put mPreviousText.isEmpty() instad of mPreviousText.length() == 0
also it would be good to put currentEventCount == 1 in some constant with self-explaining name.

Unfortunately there are cases where Aztec does only contain "non-breaking-space" char and isEmpty does return true, but the length is not 0. This is happening when you write and then remove all the content from the block.

Will address the other comments instead.

marecar3 · 2019-03-13T19:00:49Z

...ive-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java


+    @ReactProp(name = "blockType")
+    public void setBlockType(ReactAztecText view, ReadableMap inputMap) {
+        if (inputMap.hasKey("tag")) {


Maybe we can put "tag" in some constant in order to reuse it?

marecar3 · 2019-03-13T19:02:06Z

...native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java

+    // This optional variable holds the outer HTML tag that will be added to the text when the user start typing in it
+    // This is required to keep placeholder text working, and start typing with styled text.
+    // Ref: https://github.com/wordpress-mobile/gutenberg-mobile/issues/707
+    String tagName = "";


Any reason to not define it as private with prefix m?

…ard coded string

daniloercoli · 2019-03-14T08:42:46Z

I am wondering, what amount of work can it take to assign same formatting style (
)for the Placeholder like iOS has or at least to increase the initial size of RichText when type is heading, WDYT ?

The block now uses a minHeight value read from variable.scss, defined in the mobile project, this way the rending issue is mitigated a bit.
Didn't find a quick way of stylizing the hint text on Android.

…rg-mobile into issue/707-Placeholder-is-missing-on-Heading-block * 'develop' of https://github.com/wordpress-mobile/gutenberg-mobile: (22 commits) Update gutenberg ref Update gutenberg ref Update gutenberg ref Update heading placeholder size on heading level change (#742) Fix - HTML Editor - Keyboard can not be closed (#743) Update gutenberg ref Update gutenberg ref Fix lint errors Make sure onSafeAreaInsetsUpdate isn't called when BlockManager is unmounted Remove extra semicolon Remove unused editor settings prop Unmount the app after each test to prevent dangling subscribers Fix switching to html when initialHtmlModeEnabled is true Auto import native versions of variables and colors Fix lint errors Use @wordpress/edit-post package to bootstrap the editor and refactor components Add symlink for @wordpress/token-list Minor syntax fixes Update tests to use the un deprecated version of the store methods Update gutenberg ref ... # Conflicts: # gutenberg

marecar3

Nice work @daniloercoli! LGTM!

daniloercoli added 2 commits March 12, 2019 19:47

Add the property tagName to the Aztec React Native Wrapper, and use…

f532315

… it to wrap the text inserted by the user.

Update GB ref

0346d5b

daniloercoli added [Type] Bug Something isn't working [Status] Not Ready For Review [OS] Android labels Mar 12, 2019

daniloercoli requested a review from marecar3 March 12, 2019 19:04

daniloercoli added 5 commits March 12, 2019 20:13

Reuse blockType instead of adding a new property

bfe04df

Update GB hash

d320301

Add comments

26a773b

Update GB ref

46606b1

daniloercoli mentioned this pull request Mar 13, 2019

[RNmobile] Bring Placeholder text back working on Heading blocks WordPress/gutenberg#14404

Merged

daniloercoli removed the [Status] Not Ready For Review label Mar 13, 2019

marecar3 reviewed Mar 13, 2019

View reviewed changes

daniloercoli added 3 commits March 14, 2019 09:00

Make tagName private

e9b0e38

Introduce a constant for the string tag and use it instead of the h…

4954937

…ard coded string

Raise the minHeight value for heading block to 60 (it was 50)

55e4dba

daniloercoli added 4 commits March 14, 2019 09:45

Update GB ref

5e1905f

Tests: Add mock for the newly added CSS class

d57b37f

Update GB ref

8cda4bc

marecar3 approved these changes Mar 14, 2019

View reviewed changes

Update gb hash after merging the companion PR

5ce3e12

daniloercoli merged commit b8b0369 into develop Mar 14, 2019

daniloercoli deleted the issue/707-Placeholder-is-missing-on-Heading-block branch March 14, 2019 15:53

daniloercoli mentioned this pull request Mar 15, 2019

2nd Fix for Placeholder on Heading block #753

Merged

marecar3 mentioned this pull request Jul 1, 2019

Placeholder is missing on Heading block #707

Closed

cameronvoell mentioned this pull request Sep 5, 2019

Heading Block loses style after hitting backspace until empty and typing again #1355

Closed

Tug mentioned this pull request Dec 9, 2019

RNMobile: Remove unneeded/duplicate css WordPress/gutenberg#18759

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for Placeholder that is missing on heading block#739

Fix for Placeholder that is missing on heading block#739
daniloercoli merged 15 commits intodevelopfrom
issue/707-Placeholder-is-missing-on-Heading-block

daniloercoli commented Mar 12, 2019 •

edited

Loading

Uh oh!

marecar3 commented Mar 13, 2019

Uh oh!

marecar3 Mar 13, 2019

Uh oh!

daniloercoli Mar 14, 2019

Uh oh!

marecar3 Mar 13, 2019 •

edited

Loading

Uh oh!

daniloercoli Mar 14, 2019 •

edited

Loading

Uh oh!

marecar3 Mar 13, 2019

Uh oh!

daniloercoli Mar 14, 2019

Uh oh!

marecar3 Mar 13, 2019 •

edited

Loading

Uh oh!

daniloercoli commented Mar 14, 2019

)for the Placeholder like iOS has or at least to increase the initial size of RichText when type is heading, WDYT ?

Uh oh!

marecar3 left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

daniloercoli commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marecar3 commented Mar 13, 2019

Uh oh!

marecar3 Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

daniloercoli Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

marecar3 Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniloercoli Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marecar3 Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

daniloercoli Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

marecar3 Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniloercoli commented Mar 14, 2019

)for the Placeholder like iOS has or at least to increase the initial size of RichText when type is heading, WDYT ?

Uh oh!

marecar3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

daniloercoli commented Mar 12, 2019 •

edited

Loading

marecar3 Mar 13, 2019 •

edited

Loading

daniloercoli Mar 14, 2019 •

edited

Loading

marecar3 Mar 13, 2019 •

edited

Loading

marecar3 left a comment •

edited

Loading