Block editor: hooks: fix rules of hooks violations #48900
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +4 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
ellatrix
referenced
this pull request
Mar 7, 2023
* Layout blockGap: Try using classnames to support block-level gap in theme.json Try implementing partially in editor Try adding block classname to the container class to deal with specificity, remove fallback gap Add fallback gap styles rendered at root Move changes to theme.json class 'up' to 6-1 file Fix rendering block-level blockGap set in the block's attributes in the post editor Implement changes in site editor / global styles comparable to PHP changes Try moving some of the layout definitions to theme.json Move layout style generation to a separate function Implement theme.json definitions approach in the site editor, ensure styles load correctly in the post editor Remove class duplication, use classname stored in theme.json instead of hard-coded classname Support split row/column values at the block level, and include output of the legacy CSS variable for backwards compatibility Ensure CSS variable is only output if gap support is opted-in Tweak tests Whitespace fix Update test Remove duplication block class from server-rendered output, update snapshot Fix failing PHP test Attempt to fix PHP test again Manually fix snapshot Fix PHP linting issue Linting Reorganise rules in theme.json Remove dead code Render base styles and only output container classes and styles if unique values are generated Move blockGap styles in global styles to a separate getLayoutStyles function Move layout_definitions up so that it's always available to base styles Linting fixes Update test snapshot Add baseStyles output to global styles Remove test snapshot Update layout supports to return a CSS string instead of a component, add check that string is non-empty before outputting container classnames and style tags Update flex layout to only output styles if unique values are set Fix is-root-container styling in post editor Update flex/flow layouts to look up layout definitions to generate gap styling Move blockGap JS logic to a shared utility function, add tests Add test in case layoutDefinitions is undefined Add minimal tests that flex and flow layouts that don't contain non-default values return empty strings Fix rebase in 6-1.php file Consolidate JS layout classnames generation Further consolidate classname generation Implement outputting non-default layout gap for classic themes Update fallback gap logic so that block themes that opt-out of blockGap but opt-in to wp-block-styles still get flex layout gap styles Fix Columns fallback gap styles in classic themes Ensure base layout styles are available in the editor for classic themes Fix root gap value Fix linting issues Fix linting issue Add a phpunit test for outputting layout styles based on layout definitions in theme.json Add additional tests, ensure base styles are still output so that alignments continue to function Remove todo items Add layout selector regex, css declaration check Add additional logical margin properties to allow list Fix flex-wrap rule in JS version of flex layout Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Rename default_layout to global_layout_settings Rename blockGapStyles to spacingStyles Fix rebase issues Fix linting issues Fix linting again * Ensure blockGap controls are not exposed in global styles when experimental skip serializiation is used. This ensures that the Gallery block is not exposed in the global styles UI, as its blockGap values are proceeded individually at the block level. Support for the Gallery block in global styles will be looked into in future follow-ups. * Fix linting issue * Implement fallback behaviour in site editor where default flex gap is still rendered in themes without blockGap but with wp-block-styles * Remove connection to wp-block-styles so that fallback flex layout styles are always output * Update resolver class to add an empty blockGap placeholder for a block, if it provides a default blockGap value * Layout blockGap: Try using classnames to support block-level gap in theme.json Try implementing partially in editor Try adding block classname to the container class to deal with specificity, remove fallback gap Add fallback gap styles rendered at root Move changes to theme.json class 'up' to 6-1 file Fix rendering block-level blockGap set in the block's attributes in the post editor Implement changes in site editor / global styles comparable to PHP changes Try moving some of the layout definitions to theme.json Move layout style generation to a separate function Implement theme.json definitions approach in the site editor, ensure styles load correctly in the post editor Remove class duplication, use classname stored in theme.json instead of hard-coded classname Support split row/column values at the block level, and include output of the legacy CSS variable for backwards compatibility Ensure CSS variable is only output if gap support is opted-in Tweak tests Whitespace fix Update test Remove duplication block class from server-rendered output, update snapshot Fix failing PHP test Attempt to fix PHP test again Manually fix snapshot Fix PHP linting issue Linting Reorganise rules in theme.json Remove dead code Render base styles and only output container classes and styles if unique values are generated Move blockGap styles in global styles to a separate getLayoutStyles function Move layout_definitions up so that it's always available to base styles Linting fixes Update test snapshot Add baseStyles output to global styles Remove test snapshot Update layout supports to return a CSS string instead of a component, add check that string is non-empty before outputting container classnames and style tags Update flex layout to only output styles if unique values are set Fix is-root-container styling in post editor Update flex/flow layouts to look up layout definitions to generate gap styling Move blockGap JS logic to a shared utility function, add tests Add test in case layoutDefinitions is undefined Add minimal tests that flex and flow layouts that don't contain non-default values return empty strings Fix rebase in 6-1.php file Consolidate JS layout classnames generation Further consolidate classname generation Implement outputting non-default layout gap for classic themes Update fallback gap logic so that block themes that opt-out of blockGap but opt-in to wp-block-styles still get flex layout gap styles Fix Columns fallback gap styles in classic themes Ensure base layout styles are available in the editor for classic themes Fix root gap value Fix linting issues Fix linting issue Add a phpunit test for outputting layout styles based on layout definitions in theme.json Add additional tests, ensure base styles are still output so that alignments continue to function Remove todo items Add layout selector regex, css declaration check Add additional logical margin properties to allow list Fix flex-wrap rule in JS version of flex layout Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Rename default_layout to global_layout_settings Rename blockGapStyles to spacingStyles Fix rebase issues Fix linting issues Fix linting again * Reuse most of the logic from #39926 * Don't accept string values * Apply root padding only on blocks with content width * Apply linting changes * Fix rebase error * Actually fix rebase * Add global padding toggle to layout-less blocks * Support custom block padding. * Output alignfull styles only when needed. * Toggle should only appear when needed. * Add context to comments about string value support * Interpret preset padding value for negative margin * Don't show toggle on blocks without content size. * Change setting name. * Improve preset processing logic * Reset padding when no content size applies. * Update style engine class to match trunk * Fix string check * Fix preset values in the editor. * Fix string check * Remove useGlobalPadding attribute * Replace padding with spacing in function parameter * Default root padding setting to false. * Dodgy fix for Cover block * Add test for get_styles_for_block function * Check for padding setting before adding classname. * Don't output vars if setting is off * Test default output without setting enabled. * Update lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php No humorous string values allowed Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Remove empty line. Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
ellatrix
force-pushed
the
fix/rules-of-hooks-violation
branch
from
e7b05f9 to
e427d42
Compare
youknowriad
reviewed
Mar 8, 2023
| const layoutClasses = hasLayoutBlockSupport | ||
| ? useLayoutClasses( attributes, name ) | ||
| : null; | ||
| const layoutClasses = useLayoutClasses( attributes, name ); |
There was a problem hiding this comment.
If this doesn't run in trunk (fallback condition), the additional useSetting and useSelect might be having an impact on performance?
There was a problem hiding this comment.
Yes. It's still strange how a few extra select calls have such an impact.
There was a problem hiding this comment.
I don't understand why this would happen because even in this function, we have a useSelect call a few lines earlier and a useSettings call a few lines below. Also all the colour/typograpthy filters have tons of useSelect and useSettings calls.
There was a problem hiding this comment.
Somethings you can try since the useSetting('layout') is already done here is to transform this hook to a function or something to avoid doing that useSetting twice and see the impact.
There was a problem hiding this comment.
Tried this in 2919bbb.
Makes sense, every time we call this hook we already have that info anyway.
There was a problem hiding this comment.
Also, this hook signature just changed in #45299, so good to change it now.
There was a problem hiding this comment.
So it's not the useSetting call... Mysterious!
There was a problem hiding this comment.
I'm tempted to temporarily remove the position hook changes so we can cancel that out.
There was a problem hiding this comment.
Ok, with the position hooks reverted, the performance result looks better.
youknowriad
reviewed
Mar 9, 2023
| * | ||
| * @return { Array } Array of CSS classname strings. | ||
| */ | ||
| export function useLayoutClasses( blockAttributes = {}, blockName ) { | ||
| export function useLayoutClasses( |
There was a problem hiding this comment.
This is just a local API right?
youknowriad
reviewed
Mar 9, 2023
| const isPositionDisabled = useIsPositionDisabled( props ); | ||
| const showPositionControls = positionSupport && ! isPositionDisabled; | ||
| const showPositionControls = | ||
| positionSupport && ! useIsPositionDisabled( props ); |
There was a problem hiding this comment.
We can probably use ifCondition to fix this and keep the performance optimizations.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
Hooks shouldn't be called conditionally. It looks like the linting is failing because of the HoCs.
I'm fixing this in a separate PR because this could potentially degrade performance (4 additional useSelect calls per block) and want to make sure it can be tracked back to the correct commit.
I did a quick check for
? useand&& usein the code base and couldn't find any other violations.See also #48884. Worth noting that these HoCs will soon disappear.
Why?
Rules of hooks violation.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast