Skip to content

Conversation

@kyrapieterse
Copy link
Contributor

@kyrapieterse kyrapieterse commented Dec 16, 2025

Removed hardcoded toolbar button positioning and related CSS for improved compatibility with both classic and site editors. Introduced a helper to open the block sidebar in either editor, updating inspector sidebar logic to use this abstraction.

fixes #6103

Summary by CodeRabbit

  • Bug Fixes

    • Resolved toolbar positioning issues and eliminated unwanted toolbar expansion when blocks are active in the editor.
    • Improved toolbar layout handling across different editor environments.
  • Refactor

    • Enhanced block inspector sidebar opening mechanism to properly support multiple editor contexts with consistent behavior.

✏️ Tip: You can customize this high-level summary in your review settings.

Removed hardcoded toolbar button positioning and related CSS for improved compatibility with both classic and site editors. Introduced a helper to open the block sidebar in either editor, updating inspector sidebar logic to use this abstraction.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 16, 2025

Walkthrough

Three files modified to address FSE toolbar and sidebar issues: toolbar styling simplified by removing absolute positioning adjustments, toolbar initialization cleaned up by removing site-editor-specific layout calculations, and inspector sidebar opening abstracted to handle environment-aware dispatch calls (FSE vs. Classic Editor).

Changes

Cohort / File(s) Summary
Toolbar layout and styling
src/editor/toolbar-buttons/editor.scss, src/editor/toolbar-buttons/index.js
Removed explicit positioning adjustments (left, top, margin-left) for .maxi-toolbar-layout__button and eliminated site-editor-specific layout calculation logic. Added flex-grow: 0 containment rule for .edit-site-header-edit-mode__start and .editor-header__toolbar when body.maxi-blocks--active is set to prevent toolbar expansion.
Inspector sidebar environment abstraction
src/extensions/inspector/inspectorPath.js
Introduced openBlockSidebar() helper function that abstracts environment-specific sidebar opening logic (FSE vs. Classic Editor). Replaced direct dispatch('core/edit-post').openGeneralSidebar calls with the new helper in openSidebarAccordion() and openTransitions(), removing hardcoded Classic Editor sidebar references.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • inspectorPath.js — verify the new openBlockSidebar() helper correctly routes to FSE and Classic Editor sidebar opening based on getIsSiteEditor() environment check; ensure Promise handling is correct in calling functions
  • editor.scss — confirm the new flex-grow: 0 rule achieves intended toolbar sizing behavior without side effects
  • index.js — validate removal of left positioning logic does not regress toolbar placement

Possibly related PRs

  • PR #6005 — Also modifies toolbar rendering in index.js; addresses Maxi toolbar lifecycle management
  • PR #5966 — Modifies same toolbar stylesheet (editor.scss); alters .maxi-toolbar-layout__button rules and layout behavior
  • PR #6023 — Modifies inspectorPath.js with additional sidebar functionality; extends openSidebarAccordion() to accept targetSelector parameter

Suggested labels

ui, fse, enhancement, Final code review, lgtm, size:M

Suggested reviewers

  • Olekrut
  • elzadj

Poem

🐰 The toolbar hops with lighter step,
No absolute positions left behind—
Flex containment keeps things neat,
While sidebars find their place,
FSE and Classic now in tune,
Layout bugs: their final race! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description is minimal but lacks required template sections including testing methodology, test checklist completion, and verification steps outlined in the repository template. Complete the PR description with 'How Has This Been Tested?' section, checkboxes from the test checklist, and the verification checklist to match repository requirements.
Linked Issues check ❓ Inconclusive The PR partially addresses linked issue #6103 by removing hardcoded positioning (fixing extra spacing) and abstracting sidebar logic, but does not verify toolbar button functionality across blocks. Confirm that the refactoring resolves the toolbar button functionality issues mentioned in #6103 item 28, or provide additional changes if needed.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Refactor toolbar and sidebar logic for editor compatibility' clearly describes the main changes: removing hardcoded positioning and introducing a new sidebar helper for editor compatibility.
Out of Scope Changes check ✅ Passed The changes directly address the stated objectives: removing hardcoded toolbar positioning for spacing fixes, abstracting sidebar opening logic, and removing site-editor-specific adjustments for editor compatibility.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch FSE-UI-bugs

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kyrapieterse kyrapieterse self-assigned this Dec 16, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/extensions/inspector/inspectorPath.js (1)

53-62: Consider adding error handling to the Promise chain.

If openBlockSidebar() or subsequent dispatches fail, the rejection is unhandled. A .catch() at the end would improve resilience:

 export const openSidebarAccordion = (tab, accordionName, targetSelector) => {
 	openBlockSidebar()
 		.then(() =>
 			dispatch('maxiBlocks').updateInspectorPath({
 				depth: 0,
 				value: tab,
 			})
 		)
-		.then(() => openSidebar(accordionName, targetSelector));
+		.then(() => openSidebar(accordionName, targetSelector))
+		.catch(error => console.error('Failed to open sidebar accordion:', error));
 };
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4791499 and 33a697d.

📒 Files selected for processing (3)
  • src/editor/toolbar-buttons/editor.scss (1 hunks)
  • src/editor/toolbar-buttons/index.js (0 hunks)
  • src/extensions/inspector/inspectorPath.js (4 hunks)
💤 Files with no reviewable changes (1)
  • src/editor/toolbar-buttons/index.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: e2e-js-playwright
  • GitHub Check: e2e-js
  • GitHub Check: unit-js
🔇 Additional comments (3)
src/editor/toolbar-buttons/editor.scss (1)

55-60: LGTM! Clean approach to handle toolbar expansion in both editors.

The separate handling for FSE-specific selectors alongside the existing classic editor rule (line 33-35) provides proper coverage. The selectors align with WordPress's current FSE toolbar structure.

src/extensions/inspector/inspectorPath.js (2)

64-84: Changes look correct; same error handling consideration applies.

The refactoring properly uses the new openBlockSidebar() helper. The same .catch() pattern suggested above would benefit this function as well.


28-43: Well-structured environment abstraction with correctly implemented sidebar integration.

The helper cleanly encapsulates editor-specific sidebar logic. WordPress's core/edit-site store supports the openGeneralSidebar action, and both sidebar identifiers ('edit-site/block-inspector' and 'edit-post/block') are actively used throughout the codebase. The getIsSiteEditor() function is properly imported from @extensions/fse and used consistently. The Promise-based return pattern is correct per WordPress dispatch conventions.

@Olekrut Olekrut self-requested a review December 19, 2025 17:41
@elzadj elzadj merged commit e9b7b4f into master Jan 14, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FSE ui bugs

4 participants