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 :) |
SergioEstevao
added
the
Mobile App - i.e. Android or iOS
| @@ -7,7 +7,6 @@ import { | |||
| AlignmentToolbar, | |||
There was a problem hiding this comment.
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.
It depends on how the block was implemented, for example heading now shares the same edit code as the web, so no changes needed.
| import InspectorControls from '../components/inspector-controls'; | ||
| import { getBlockDOMNode } from '../utils/dom'; | ||
|
|
||
| export default function ColorPanel( { colorSettings, clientId } ) { |
There was a problem hiding this comment.
I this this should live in block-editor/src/components instead.
There was a problem hiding this comment.
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.
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? |
SergioEstevao
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
| const tagName = 'h' + level; | ||
|
|
||
| const styles = { | ||
| color: style && style.color && style.color.text | ||
| }; |
There was a problem hiding this comment.
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.
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.
@youknowriad can you help us with that?
There was a problem hiding this comment.
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.
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.
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.
@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.
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.
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: