-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add color hooks for the mobile implementation. #21257
Conversation
Size Change: +48 B (0%) Total Size: 866 kB
ℹ️ View Unchanged
|
I was actually trying to setup a mobile env to do this. You saved me a full day of hassle downloading xcode, android studio... :) Thanks |
I think you should do it anyway :) |
@@ -7,7 +7,6 @@ import { | |||
AlignmentToolbar, |
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 new hook is implemented in several blocks. I guess we might need similar removals in other blocks (heading, group, columns, media & text)
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.
It depends on how the block was implemented, for example heading now shares the same edit code as the web, so no changes needed.
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.
code wise, this looks good.
import InspectorControls from '../components/inspector-controls'; | ||
import { getBlockDOMNode } from '../utils/dom'; | ||
|
||
export default function ColorPanel( { colorSettings, clientId } ) { |
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.
I this this should live in block-editor/src/components
instead.
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.
We have so many color related components there that need some cleanup, maybe we should avoid a new reusable component there for now. Potentially the hooks/color
could be a folder?
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.
Code looks good.
Given that we're also missing those 2 lines on the native version of the block editor hooks:
import { AlignmentHookSettingsProvider } from './align';
import './anchor';
I wouldn't be surprised if we also had inconsistencies between web and mobile around those attributes.
Code looks good, however, I'm afraid of merging that since if the gutenberg-mobile will be released before the wordpress.com release, it will break custom colors in all paragraphs after the page is opened on mobile. So let's say the gutenberg-mobile is released with that change but the wordpress.com still uses the old version. The user creates the page on the web with custom colors in paragraphs, then opens the page on mobile and loses all colors on the web because mobile app will migrate custom colors to the new system. |
While I agree with you those changes look older and still don't have an impact on mobile, the only use of the |
No worries, the new release will ship with WP mobile app 14.6 which will be released at Apr 20th. |
@SergioEstevao Could you explain a bit about real life implications of this PR? What are we fixing exactly? Are we just preventing the loss of the new What else should we do to make color rendering work as it did before? see wordpress-mobile/gutenberg-mobile#2068 |
I'm more concerned with the hooks registered in those 2 files. But then if we haven't noticed any unwanted behavior I guess we can investigate those separately 👍 |
We are using |
The change discarded the use of the TextColor HOC that I believe you were using to pass the colors down from blocks to the RichText component. The only simple way to update your code is to implement something similar to what TextColor was doing and use it on the native paragraph edit code. Maybe @youknowriad as a better suggestion? |
The "style" attribute is going to be something widely used across blocks in a very generic and defined way. It just generates CSS variables, I wonder if there's a way to "support" CSS variables on mobile somehow which would solve the issue for all these customizations? |
const tagName = 'h' + level; | ||
|
||
const styles = { | ||
color: style && style.color && style.color.text | ||
}; |
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.
Can we do this in a more generic way, in a hook or something because this will apply to all blocks with that flag enabled?
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.
I am not sure we know how to do that. Could you maybe explain with some sample code?
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.
@youknowriad can you help us with that?
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.
Right now the "style" prop is injected into the Block Wrapper using getEditWrapperProps in the hook for the web. Can we do the same for mobile to inject styles
?
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 difference here would be that we should run the "save" logic in the "edit" one because the props will be different on mobile.
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.
That link was for the color hook, here's the one for the style one https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/hooks/style.js#L118-L125
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.
@youknowriad I don't think we are in a position in native to do this like that, we will need to change our components to always read from styles props and this is not happening at the moment. I think for now we should merge this PR as it is to solve our breakages and then try to do follow up PRs on updating our native code to better support this generic styles in a more generic way.
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.
I'm not suggesting it should be solved now but mking getEditWrapperProps
work on mobile is key yes.
We don't have a way to declare selectors and inherit styles from a generic place, we need to pass each style object to the component explicitly. |
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.
Color issues are fixed! Thank you all! @SergioEstevao @dratwas @Tug
# Conflicts: # packages/block-editor/src/hooks/color.js
@SergioEstevao I saw you merged |
# Conflicts: # packages/block-library/src/heading/edit.js
# Conflicts: # packages/block-library/src/heading/edit.js
Description
Updates the mobile implementation to support the new colors hooks, effect and filters.
How has this been tested?
Screenshots
Types of changes
Checklist: