Skip to content

Conversation

@James-Baloyi
Copy link
Contributor

@James-Baloyi James-Baloyi commented Oct 30, 2025

Summary by CodeRabbit

  • New Features

    • Added edit mode visual styling for menu components.
  • Style

    • Improved horizontal menu scrolling behavior with CSS-based approach.
    • Refined menu item styling with better child element targeting.
    • Updated default menu appearance with simplified borders and corner styling.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

Walkthrough

This pull request refactors the menu component styling architecture by removing the style prop from the LayoutMenu component's public API, replacing it with CSS-based horizontal scrolling and direct-child selectors. It introduces edit-mode styling, updates color derivation logic in the horizontalMenu designer, and simplifies default border and radius styles.

Changes

Cohort / File(s) Summary
Menu component API removal
shesha-reactjs/src/components/menu/index.tsx
Removed the style prop from the IProps interface and component destructuring; removed style argument from useHorizontalMenuDropdownStyles hook call.
Menu styles refactoring
shesha-reactjs/src/components/menu/styles.ts
Replaced overflow handling with CSS-based scroll setup; introduced direct-child selectors (> .menu-submenu-active, > .menu-item-selected) for submenu and item styling; added editMode CSS block disabling pointer events; expanded GlobalMenuStyles to apply styleOnSubMenu across dropdown contexts; exposed editMode in useStyles public API.
Menu dropdown styles refactoring
shesha-reactjs/src/components/menu/useHorizontalMenuDropdownStyles.ts
Removed style prop from UseHorizontalMenuDropdownStylesProps; consolidated hover and selected state styling under styleOnHover and styleOnSelected; replaced style-dependent CSS logic with direct styleOnSubMenu application; updated dependency array cleanup.
HorizontalMenu designer component
shesha-reactjs/src/designer-components/horizontalMenu/index.tsx
Wrapped LayoutMenu in div with "edit-mode" class in edit mode; updated color derivation (removed implicit itemColor/itemBackground, added derivation from model.font?.color and model.background.color).
HorizontalMenu default styles
shesha-reactjs/src/designer-components/horizontalMenu/utils.ts
Changed default border from 1px solid to 0px/none/transparent; changed corner radii from 8 to 0.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • CSS refactoring in styles.ts: Multiple selector changes with direct-child targeting require careful verification of styling hierarchy and cascade behavior
  • API changes across multiple files: Style prop removal affects public interfaces in index.tsx, useHorizontalMenuDropdownStyles.ts, and styles.ts—verify no breaking changes in dependent code
  • Color derivation logic updates: Conditional logic changes in horizontalMenu/index.tsx for itemColor and itemBackground derivation require tracing through design intent
  • Edit-mode styling integration: New editMode CSS and wrapper div logic needs validation across edit and preview modes

Possibly related PRs

  • James/en/3459 #4048: Directly related—modifies the same menu components' public styling API (LayoutMenu and useHorizontalMenuDropdownStyles); one removes the style prop while the other adds/manages styleOnHover and styleOnSelected.

Suggested reviewers

  • AlexStepantsov
  • czwe-01

Poem

🐰 A menu styled with grace so fine,
Direct-child selectors now align,
No borders round, no styles passed down,
Edit mode hops into town,
Scrolling smooth through CSS's design!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The pull request title "styles updates" is vague and generic, similar to the "misc updates" example provided in the evaluation criteria. While the changeset does involve style-related modifications across multiple files (styles.ts, menu component updates, and defaultStyles adjustments), the title does not meaningfully communicate what was specifically changed or why. A developer scanning the commit history would have no clear understanding of the primary intent—whether this is a refactoring effort, an API change, a bug fix, or a feature addition. The title lacks specificity and clarity needed for effective code history documentation. Consider revising the title to be more specific and descriptive of the main changes. For example, a title like "Remove style prop from LayoutMenu API and refactor menu styling logic" or "Refactor menu component styling and add edit-mode support" would better communicate the purpose of the changeset. The new title should clearly indicate the primary objective or the most significant change from a developer's perspective.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

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: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8d7d356 and 0841f4f.

📒 Files selected for processing (5)
  • shesha-reactjs/src/components/menu/index.tsx (0 hunks)
  • shesha-reactjs/src/components/menu/styles.ts (9 hunks)
  • shesha-reactjs/src/components/menu/useHorizontalMenuDropdownStyles.ts (2 hunks)
  • shesha-reactjs/src/designer-components/horizontalMenu/index.tsx (2 hunks)
  • shesha-reactjs/src/designer-components/horizontalMenu/utils.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • shesha-reactjs/src/components/menu/index.tsx
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-28T14:21:55.782Z
Learnt from: teboho
PR: shesha-io/shesha-framework#4084
File: shesha-reactjs/src/components/quickView/styles/styles.ts:41-94
Timestamp: 2025-10-28T14:21:55.782Z
Learning: In shesha-reactjs/src/components/quickView/styles/styles.ts, the quickViewContent styles intentionally use multiple `!important` flags and broad selectors (including spans, form items, inputs, links, and labels) to ensure proper targeting and prevent layout breakage in the quick view component.

Applied to files:

  • shesha-reactjs/src/components/menu/useHorizontalMenuDropdownStyles.ts
  • shesha-reactjs/src/designer-components/horizontalMenu/index.tsx
  • shesha-reactjs/src/components/menu/styles.ts
  • shesha-reactjs/src/designer-components/horizontalMenu/utils.ts
📚 Learning: 2025-05-28T07:55:21.036Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3312
File: shesha-reactjs/src/hooks/formComponentHooks.ts:183-192
Timestamp: 2025-05-28T07:55:21.036Z
Learning: In shesha-reactjs formComponentHooks.ts, the background style initialization deliberately sets valid values from properties panel immediately to provide better UX, rather than waiting for the useDeepCompareEffect to run. This intentional pattern prevents a flash of no background and shows the background image immediately when the component renders.

Applied to files:

  • shesha-reactjs/src/designer-components/horizontalMenu/index.tsx
  • shesha-reactjs/src/components/menu/styles.ts
  • shesha-reactjs/src/designer-components/horizontalMenu/utils.ts
📚 Learning: 2025-06-12T16:55:57.638Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.

Applied to files:

  • shesha-reactjs/src/designer-components/horizontalMenu/index.tsx
  • shesha-reactjs/src/components/menu/styles.ts
  • shesha-reactjs/src/designer-components/horizontalMenu/utils.ts
📚 Learning: 2025-10-02T11:25:33.370Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3926
File: shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts:138-166
Timestamp: 2025-10-02T11:25:33.370Z
Learning: In the shesha-framework repository, the hint popover components (sha-quick-search-hint-popover, sha-table-view-selector-hint-popover, sha-table-pager-hint-popover) in shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts intentionally use hard-coded color `rgb(214, 214, 214)` with `!important` to impose a specific consistent color across themes, overriding theme tokens by design.

Applied to files:

  • shesha-reactjs/src/designer-components/horizontalMenu/index.tsx
  • shesha-reactjs/src/components/menu/styles.ts
📚 Learning: 2025-06-26T19:59:22.872Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:1280-1280
Timestamp: 2025-06-26T19:59:22.872Z
Learning: In shesha-reactjs chart settings forms, the styling panels (border, shadow, background) should not include buttonType-based hidden conditions since charts are not button components. The border configuration should follow the standard component pattern without button-specific restrictions.

Applied to files:

  • shesha-reactjs/src/components/menu/styles.ts
  • shesha-reactjs/src/designer-components/horizontalMenu/utils.ts
📚 Learning: 2025-06-10T11:52:51.462Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3374
File: shesha-reactjs/src/generic-pages/settings/dynamic-theme/textsExample.tsx:13-15
Timestamp: 2025-06-10T11:52:51.462Z
Learning: In the Shesha framework's dynamic theme system, the "Global text" Typography element in textsExample.tsx is intentionally left without explicit styling to demonstrate automatic global text color application through the ConfigProvider's colorText token, while the "Default text" element uses explicit theme color styling to show the difference between global and default text color approaches.

Applied to files:

  • shesha-reactjs/src/components/menu/styles.ts
🧬 Code graph analysis (1)
shesha-reactjs/src/designer-components/horizontalMenu/index.tsx (2)
shesha-reactjs/src/components/menu/index.tsx (1)
  • LayoutMenu (52-289)
shesha-reactjs/src/providers/form/utils.ts (1)
  • getStyle (1199-1208)
⏰ 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). (1)
  • GitHub Check: build-attempt

Comment on lines +138 to +155
<div className="edit-mode">
<LayoutMenu
colors={colors}
fontSize={typeof fontSize === 'string' ? fontSize : String(fontSize)}
padding={{ x: gap, y: height }}
style={{
...finalStyle,
...finalFontStyles,
width: width,
} as React.CSSProperties}
styleOnHover={getStyle(model?.styleOnHover, data)}
styleOnSelected={getStyle(model?.styleOnSelected, data)}
styleOnSubMenu={getStyle(model?.styleOnSubMenu, data)}
overflow={model.overflow}
width={width}
fontStyles={finalFontStyles as React.CSSProperties}
menuId={model.id}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Restore runtime interactivity by gating edit-mode.

With the new global rule .edit-mode { pointer-events: none; } (see styles.ts), wrapping LayoutMenu in an unconditional <div className="edit-mode"> disables every click/hover/scroll on the menu—even outside the designer—so navigation breaks immediately. Please apply that class only when the component is actually in edit mode (or drop it entirely).

Apply this diff to gate the class on the edit state:

-              <div className="edit-mode">
+              <div className={componentState.isInEditMode ? 'edit-mode' : undefined}>

(Be sure to use the correct flag for your configurable renderer’s edit-state.)

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/horizontalMenu/index.tsx around lines
138 to 155, the LayoutMenu is wrapped unconditionally with a
div.className="edit-mode" which disables all pointer events due to the global
.edit-mode { pointer-events: none } rule; change this so the class is only
applied when the renderer is actually in edit state (e.g. conditionally set
className={isEditMode ? 'edit-mode' : undefined} or remove the wrapper
entirely), using the correct configurable renderer edit-state flag available in
this component so runtime interactivity (click/hover/scroll) is preserved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant