Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[RNMobile] Update "add block" button style in default editor view #39726
[RNMobile] Update "add block" button style in default editor view #39726
Changes from 16 commits
2b3c19d
37960ce
5ad815a
556ac19
53db20c
392832f
b0693a7
8197083
46c31dd
03aa6ae
532c437
b7ac622
d699e78
38df61c
4af6403
005e6c7
4b5aa48
19e5eb5
ede9c16
0212fbf
c31c897
d8684a2
0039c10
7769cac
4a2ed4a
c459a7b
d2822fe
82eeb5f
7d3399f
fd73fa6
db5d32e
a35a8e5
3715ee6
ac5607e
95cda71
24d9294
adf4173
bc9b4ea
86fada7
28e772b
ee54130
ee12890
fc2403f
4106e33
1820356
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
👋 @SiobhyB , not sure if you have tried this already for fixing the button flashing issue, but wanted to share some of my findings: a bit of promising success by using the
shouldReplaceBlock
state variable in the Inserter class. Specifically, by adding a&& ! shouldReplaceBlock
conditional. This way, the text label is not shown when the editor is in the process of replacing the default richtext block.But as I said, that's half-success, since the toolbar styling is not correct, since that's governed by the
HeaderToolbar
component. Not sure yet how to pass such transitional state to the HeaderToolbar yet.Hope this all helps!
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.
@hypest, thank you so much for this! 🙇♀️
I think you're right that the trickiest part to this is with updating the parent component. With that in mind, I've made a change to set
isDefaultView
as an editor setting, which I envisage as being similar to other editor settings that influence the editor's appearance, such asfixedToolbar
,reducedUI
,focusMode
, etc.I've been able to get some code that's functionally working with that approach (which I've committed to this PR) but it still has some issues. The biggest of which is: As my latest code changes involve an import of
@wordpress/editor
inside a@wordpress/block-editor
file, the automated tests fail with theCannot read property 'SETTINGS_DEFAULTS' of undefined, js engine: hermes
error (same issue that ended up blocking this PR: #35021).I feel like this is on the right track, though, and it's a case of finding a better way to control the new setting's data. I'll keep looking into this, but want to be transparent that I don't think it's something that will be quick for me to get to the bottom of.
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.
Hmm, feels to me that the this showing/hiding of the textual part of the inserter button is not a setting per se. I mean, it's not configuration that we are expecting to hold through the lifecycle of the editor instance. Instead, it's state that changes depending on the user/caret focus. Are there other similar examples already in place perhaps, where we use the settings for state like this?
Perhaps what we need to "copy" is the way the settings are implemented (hooks/events/etc) but not piggyback on the settings component itself.
That said, I wonder if @jhnstn you have thoughts on this?
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.
The only other one that comes to mind is that we use the settings to update
impressions
, which is then used to update parts of the native UI. It was seeing this line of code in the Inserter component that got me thinking of using the approach I've taken in the latest commits. It feels kind of similar to me in that theimpressions
are more of a state than a setting.I wonder if there might be a way to use
context
to change this data. 🤔 Looking forward to hearing any ideas from Jason!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.
This issue was addressed in bc9b4ea.