enhance: doc nav block individual nav drop shadow #245
Conversation
…individual_nav_drop_shadow
WalkthroughAdds a new Doc Navigation Gutenberg block (editor UI, block metadata, styles, SSR PHP renderer and registration), a PHP helper for doc prev/next navigation, a compiled data store for docs/settings, webpack runtime additions in built JS, CSS source-map/banner updates, and several updated asset fingerprints and dependencies (adds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🪛 Biome (2.1.2)assets/build/index.css[error] 2206-2206: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally. grid-template-columns is already defined here. Remove or rename the duplicate property to ensure consistent styling. (lint/suspicious/noDuplicateProperties) [error] 6188-6188: Unexpected value or character. Expected one of: (parse) [error] 6188-6188: Unexpected value or character. Expected one of: (parse) [error] 6188-6188: Unexpected value or character. Expected one of: (parse) [error] 6188-6188: Unexpected value or character. Expected one of: (parse) [error] 6188-6188: Unexpected value or character. Expected one of: (parse) [error] 6188-6188: Unexpected shorthand property flex-flow after flex-direction (lint/suspicious/noShorthandPropertyOverrides) [error] 6188-6188: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally. display is already defined here. Remove or rename the duplicate property to ensure consistent styling. (lint/suspicious/noDuplicateProperties) [error] 6188-6188: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally. display is already defined here. Remove or rename the duplicate property to ensure consistent styling. (lint/suspicious/noDuplicateProperties) [error] 6188-6188: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally. transition is already defined here. Remove or rename the duplicate property to ensure consistent styling. (lint/suspicious/noDuplicateProperties) 🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/blocks/DocNavigation/index.js (1)
2-2: Potentially unused import.The
__function from@wordpress/i18nis imported but doesn't appear to be used in this file. It's likely used in the Edit component instead.Apply this diff to remove the unused import:
-import { __ } from '@wordpress/i18n';src/blocks/DocNavigation/style.scss (2)
10-67: Layout correctly supports individual block shadows.The flex layout with
space-betweenensures the previous and next navigation blocks are separate flex items, which aligns with the PR objective of applying shadows to individual blocks rather than the full-width container.However, the extensive use of
!importantflags (lines 13-17, 21-24) may indicate specificity conflicts and could complicate future maintenance.Consider reducing
!importantusage by:
- Increasing selector specificity naturally (e.g.,
.wedocs-document .wedocs-doc-navigation)- Reviewing what styles are being overridden
- Using more specific selectors instead of forcing precedence
This would improve maintainability without affecting functionality.
1-3: Limit Tailwind imports to utilities only
Sinceedit.jsuses classes likeflexandjustify-between, keep@tailwind utilitiesbut remove@tailwind baseand@tailwind componentsto avoid importing unused resets and components insrc/blocks/DocNavigation/style.scss.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
assets/build/block.js.mapis excluded by!**/*.mapassets/build/frontend.css.mapis excluded by!**/*.mapassets/build/frontend.js.mapis excluded by!**/*.mapassets/build/index.css.mapis excluded by!**/*.mapassets/build/index.js.mapis excluded by!**/*.mapassets/build/print.css.mapis excluded by!**/*.mapassets/build/print.js.mapis excluded by!**/*.mapassets/build/store.js.mapis excluded by!**/*.mapassets/build/style-block.css.mapis excluded by!**/*.map
📒 Files selected for processing (22)
assets/build/block.asset.php(1 hunks)assets/build/blocks/DocNavigation/block.json(1 hunks)assets/build/blocks/DocNavigation/render.php(1 hunks)assets/build/frontend.asset.php(1 hunks)assets/build/frontend.css(2 hunks)assets/build/frontend.js(1 hunks)assets/build/index.asset.php(1 hunks)assets/build/index.css(7 hunks)assets/build/print.asset.php(1 hunks)assets/build/print.css(2 hunks)assets/build/print.js(1 hunks)assets/build/store.asset.php(1 hunks)assets/build/store.js(1 hunks)includes/functions.php(1 hunks)src/blocks/DocNavigation/block.json(1 hunks)src/blocks/DocNavigation/edit.js(1 hunks)src/blocks/DocNavigation/index.js(1 hunks)src/blocks/DocNavigation/render.php(1 hunks)src/blocks/DocNavigation/save.js(1 hunks)src/blocks/DocNavigation/style.scss(1 hunks)src/blocks/index.js(1 hunks)wedocs.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/blocks/DocNavigation/render.php (2)
src/blocks/DocNavigation/edit.js (1)
attributes(17-43)includes/functions.php (1)
wedocs_get_doc_navigation_posts(191-238)
assets/build/blocks/DocNavigation/render.php (2)
src/blocks/DocNavigation/edit.js (1)
attributes(17-43)includes/functions.php (1)
wedocs_get_doc_navigation_posts(191-238)
src/blocks/DocNavigation/index.js (1)
src/blocks/DocNavigation/edit.js (1)
Edit(13-442)
🪛 Biome (2.1.2)
assets/build/store.js
[error] 321-324: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 325-325: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 375-375: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 376-376: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
assets/build/index.css
[error] 2191-2191: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
grid-template-columns is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 5943-5943: Unexpected value or character.
Expected one of:
(parse)
[error] 5943-5943: Unexpected value or character.
Expected one of:
(parse)
[error] 5943-5943: Unexpected value or character.
Expected one of:
(parse)
[error] 5943-5943: Unexpected value or character.
Expected one of:
(parse)
[error] 5943-5943: Unexpected value or character.
Expected one of:
(parse)
[error] 5943-5943: Unexpected shorthand property flex-flow after flex-direction
(lint/suspicious/noShorthandPropertyOverrides)
[error] 5943-5943: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
display is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 5943-5943: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
display is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 5943-5943: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
transition is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🪛 PHPMD (2.15.0)
src/blocks/DocNavigation/render.php
9-9: Avoid unused parameters such as '$content'. (undefined)
(UnusedFormalParameter)
38-38: Avoid unused local variables such as '$wpdb'. (undefined)
(UnusedLocalVariable)
61-61: Avoid unused local variables such as '$wp_class_name'. (undefined)
(UnusedLocalVariable)
assets/build/blocks/DocNavigation/render.php
9-9: Avoid unused parameters such as '$content'. (undefined)
(UnusedFormalParameter)
38-38: Avoid unused local variables such as '$wpdb'. (undefined)
(UnusedLocalVariable)
61-61: Avoid unused local variables such as '$wp_class_name'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (12)
assets/build/frontend.asset.php (1)
1-1: Version hash update confirmedBundle metadata bump is consistent with the rebuilt frontend asset; no further action needed.
assets/build/print.asset.php (1)
1-1: Print asset hash refresh looks fineFingerprint aligns with regenerated print bundle; dependencies remain unchanged and correct.
assets/build/store.asset.php (1)
1-1: Store asset metadata updates verifiedVersion bump reflects the rebuilt store bundle while dependency declarations stay accurate.
assets/build/index.asset.php (1)
1-1: Index asset fingerprint update acknowledgedThe new hash matches the refreshed editor bundle; dependency list remains correct.
assets/build/print.css (1)
1-186: Bundler banner and source map reference look goodGenerated header and
sourceMappingURLmarker are expected outputs from the CSS build and require no follow-up.src/blocks/index.js (1)
3-3: LGTM!The import statement follows the established pattern and correctly loads the new DocNavigation block module.
wedocs.php (2)
50-50: LGTM!The require_once statement correctly loads the DocNavigation render file, following the same pattern as the existing DocsGrid block.
187-193: LGTM!The block registration follows the established pattern and correctly registers the DocNavigation block with its server-side render callback.
src/blocks/DocNavigation/save.js (1)
1-4: LGTM!The save function correctly returns null for server-side rendering, following WordPress Gutenberg best practices for SSR blocks. The comment clearly explains the behavior.
src/blocks/DocNavigation/index.js (1)
7-17: LGTM!The block registration is correctly structured with the Edit component for the editor view and a null save function for server-side rendering. The JSDoc comments provide helpful references.
src/blocks/DocNavigation/style.scss (2)
5-8: Panel styling adjustment is appropriate.This targets a specific WordPress editor component to adjust spacing in the block inspector controls. The high specificity is necessary to override default WordPress styles.
69-85: LGTM!The responsive adjustments appropriately stack the navigation blocks vertically on mobile while maintaining the individual block structure. This ensures the shadow styling (applied per block) works correctly across all screen sizes.
| $wrapper_attributes = get_block_wrapper_attributes(); | ||
|
|
There was a problem hiding this comment.
Fix duplicate wrapper attributes to keep core block classes.
Line 55 defines $wrapper_attributes = get_block_wrapper_attributes();, but Line 246 renders <div class="…" <?php echo $wrapper_attributes; ?>>. This emits two class attributes. Browsers drop the second one, so WordPress’ wrapper classes (wp-block-*, className, spacing styles, etc.) never land in the DOM. That breaks block supports and any consumer expecting those classes. Remove the manual class attribute and pass your custom class into get_block_wrapper_attributes() instead.
- $wrapper_attributes = get_block_wrapper_attributes();
+ $wrapper_attributes = get_block_wrapper_attributes(
+ [ 'class' => 'wedocs-document wedocs-doc-navigation-preview' ]
+ );
...
- <div class="wedocs-document wedocs-doc-navigation-preview" <?php echo $wrapper_attributes; ?>>
+ <div <?php echo $wrapper_attributes; ?>>Also applies to: 246-247
🤖 Prompt for AI Agents
In assets/build/blocks/DocNavigation/render.php around lines 55 and 246-247, you
currently call $wrapper_attributes = get_block_wrapper_attributes(); and then
render a div with a manual class attribute plus <?php echo $wrapper_attributes;
?> which produces duplicate class attributes; remove the manual class attribute
from the div and instead pass your custom class(es) into
get_block_wrapper_attributes() (e.g. merge your class string via the 'class' key
or appropriate argument) so the function outputs a single attribute string
containing both core WP block classes and your custom classes.
| display: grid; | ||
| grid-template-columns: repeat(1, minmax(0, 1fr)); | ||
| grid-template-columns: auto; | ||
| grid-template-rows: repeat(2, minmax(0, 1fr)); | ||
| grid-template-rows: 40px 1fr; | ||
| place-items: center; |
There was a problem hiding this comment.
Resolve duplicate grid-template declarations
The .steps .step block defines grid-template-columns and grid-template-rows twice in succession. The second assignments override the first ones, leaving dead declarations and tripping Biome (lint/suspicious/noDuplicateProperties) on the build. Please drop the redundant lines so the generated CSS stays clean and the linter passes. For example:
.steps .step {
display: grid;
- grid-template-columns: repeat(1, minmax(0, 1fr));
- grid-template-columns: auto;
- grid-template-rows: repeat(2, minmax(0, 1fr));
- grid-template-rows: 40px 1fr;
+ grid-template-columns: auto;
+ grid-template-rows: 40px 1fr;
place-items: center;
text-align: center;
min-width: 4rem;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| display: grid; | |
| grid-template-columns: repeat(1, minmax(0, 1fr)); | |
| grid-template-columns: auto; | |
| grid-template-rows: repeat(2, minmax(0, 1fr)); | |
| grid-template-rows: 40px 1fr; | |
| place-items: center; | |
| .steps .step { | |
| display: grid; | |
| grid-template-columns: auto; | |
| grid-template-rows: 40px 1fr; | |
| place-items: center; | |
| text-align: center; | |
| min-width: 4rem; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 2191-2191: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
grid-template-columns is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🤖 Prompt for AI Agents
In assets/build/index.css around lines 2189 to 2194, the .steps .step block
contains duplicate declarations for grid-template-columns and grid-template-rows
(first using repeat(...) and then overridden by explicit values); remove the
redundant lines (either the repeat(...) lines or the explicit auto/40px 1fr
lines depending on the intended layout) so each property appears only once,
leaving the correct grid-template-columns and grid-template-rows declaration and
eliminating the duplicate-property lint error.
| const { | ||
| restrictedArticleList | ||
| } = state; | ||
| return restrictedArticleList.find(article => article.id === id); | ||
| } |
There was a problem hiding this comment.
Fix resolver signature to avoid runtime TypeError
WordPress resolvers only receive the selector arguments. Here the first argument is the requested id, not the store state, so destructuring restrictedArticleList throws TypeError when this resolver runs. That breaks any consumer calling select( 'wedocs/docs' ).getRestrictedArticle( id ). Refactor to fetch (or otherwise obtain) the data without treating the first argument as state.
- *getRestrictedArticle(state, id) {
- const {
- restrictedArticleList
- } = state;
- return restrictedArticleList.find(article => article.id === id);
- }
+ *getRestrictedArticle(id) {
+ const restrictedArticles = yield _actions__WEBPACK_IMPORTED_MODULE_0__["default"].fetchFromAPI('/wp/v2/docs/meta?key=wedocs_restrict_admin_article_access');
+ yield _actions__WEBPACK_IMPORTED_MODULE_0__["default"].setRestrictedArticles(restrictedArticles);
+ const restrictedArticle = restrictedArticles.find(article => article.id === id);
+ if (restrictedArticle) {
+ yield _actions__WEBPACK_IMPORTED_MODULE_0__["default"].setRestrictedArticle(restrictedArticle);
+ }
+ return restrictedArticle;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In assets/build/store.js around lines 456-460, the resolver currently treats its
first parameter as the store state and destructures restrictedArticleList from
it, but WordPress resolvers receive only the selector args (the first arg is the
requested id), causing a TypeError; update the resolver signature to accept (id)
and obtain restrictedArticleList from the store/registry state instead (e.g.,
call the store/registry getter or select the store to read
restrictedArticleList), then perform the .find on that list and handle the case
where the list is undefined before returning the matched article.
| $wrapper_attributes = get_block_wrapper_attributes(); | ||
|
|
||
| // Extract WordPress style system attributes |
There was a problem hiding this comment.
Fix duplicate wrapper classes.
get_block_wrapper_attributes() already emits a class="" attribute. Adding another class="" before echoing it means the browser keeps only the second one (the wrapper classes) and drops wedocs-document wedocs-doc-navigation-preview, so our block loses the styling hooks that keep the two navigation cards visually distinct—undoing the drop-shadow enhancement. Please merge those classes into the wrapper attributes instead of declaring a separate class attribute.
- $wrapper_attributes = get_block_wrapper_attributes();
+ $wrapper_attributes = get_block_wrapper_attributes(
+ [
+ 'class' => 'wedocs-document wedocs-doc-navigation-preview',
+ ]
+ );
…
- <div class="wedocs-document wedocs-doc-navigation-preview" <?php echo $wrapper_attributes; ?>>
+ <div <?php echo $wrapper_attributes; ?>>Also applies to: 246-248
🤖 Prompt for AI Agents
In src/blocks/DocNavigation/render.php around lines 55-57 (and similarly lines
246-248), the code outputs a separate class="" before echoing
get_block_wrapper_attributes(), which causes duplicate class attributes and the
browser to drop the first set (losing wedocs-document
wedocs-doc-navigation-preview). Merge the additional classes into the attributes
returned by get_block_wrapper_attributes() instead of emitting a second class
attribute: parse or append your custom classes onto the wrapper attributes
string/array (or supply them via
get_block_wrapper_attributes(['class'=>'your-classes']) if supported) and then
echo the single combined wrapper attributes so both sets of classes are
preserved.
|
Please resolve conflict of the pr @sapayth vai |
…block_individual_nav_drop_shadow
2 participants
fixes: #233
Added shadow option for the doc navigations. Added WP default shadow and also custom shadow settings with horizontal and vertical offset, blur radius, spread radius, custom color, opacity, inset etc.
Summary by CodeRabbit