-
Notifications
You must be signed in to change notification settings - Fork 58
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 for Placeholder that is missing on heading block #739
Fix for Placeholder that is missing on heading block #739
Conversation
… 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
Nice work @daniloercoli! While I was testing this PR on my Nexus 5x, Android: 8.0, I have noticed some glitch. I am wondering, what amount of work can it take to assign same formatting style ( |
|
||
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -504,6 +513,11 @@ public void onTextChanged(CharSequence s, int start, int before, int count) { | |||
oldText, | |||
start, | |||
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())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -302,6 +303,13 @@ public void setColor(ReactAztecText view, @Nullable Integer color) { | |||
view.setTextColor(newColor); | |||
} | |||
|
|||
@ReactProp(name = "blockType") | |||
public void setBlockType(ReactAztecText view, ReadableMap inputMap) { | |||
if (inputMap.hasKey("tag")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can put "tag"
in some constant in order to reuse it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
// 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 = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not define it as private with prefix m
?
The block now uses a |
…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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @daniloercoli! LGTM!
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
Test 2
Test 3