Adjustments after review#171
Open
gkrajniak wants to merge 8 commits into
Open
Conversation
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR centralizes dashboard breakpoint logic from hardcoded media queries into TypeScript and SCSS constants, migrates to container queries for responsive layout, makes section column defaults optional to inherit responsive defaults, and improves card editing UX with grab/grabbing cursor feedback during drag operations. ChangesDashboard Responsive Layout & Card Editing UX
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/ngx/declarative-ui/stories/dashboard.stories.ts (1)
48-60: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winEliminate duplication of
table-podscard configuration.The
table-podscard configuration is duplicated between this file (AVAILABLE_CARDS) anddashboard.cards.ts(CARDSarray). This duplication required synchronized updates in this PR and creates maintenance risk—future changes must be applied in both locations or the fixtures will drift.Consider extracting the shared configuration to a constant or factory function that both arrays can reference. The
labelfield can be added conditionally where needed.♻️ Suggested approach
In
dashboard.cards.ts, extract a shared factory:export const createTablePodsCard = (overrides?: Partial<CardConfig>): CardConfig => ({ id: 'table-pods', w: 12, h: 45, component: 'mfp-wc-declarative-table-card', componentInputs: { config: TABLE_CARD_CONFIG, header: 'Pods', headerTooltip: 'This table lists all pods running in the cluster.', resources: TABLE_RESOURCES, }, ...overrides, });Then use it in both files:
// In dashboard.cards.ts export const CARDS: CardConfig[] = [ // ... other cards createTablePodsCard(), ]; // In dashboard.stories.ts const AVAILABLE_CARDS: CardConfig[] = [ createTablePodsCard({ label: 'Pods Table' }), // ... other cards ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/ngx/declarative-ui/stories/dashboard.stories.ts` around lines 48 - 60, The table-pods card config is duplicated between AVAILABLE_CARDS and the CARDS array; extract the shared object into a factory or constant (e.g., createTablePodsCard or TABLE_PODS_CONFIG) that returns the base CardConfig for id 'table-pods' (including w, h, component, componentInputs with TABLE_CARD_CONFIG/TABLE_RESOURCES and header/headerTooltip), then replace the inline definition in AVAILABLE_CARDS with a call to that factory and pass { label: 'Pods Table' } as an override where the label is only needed; update CARDS to use the same factory without the label to remove duplication.
🧹 Nitpick comments (2)
projects/ngx/declarative-ui/dashboard/card/dashboard-card.component.scss (1)
51-68: ⚡ Quick winConsider consolidating drag-state rules.
The three
:host-context(.ui-draggable-dragging)rule blocks (lines 54-56, 61-64, 66-68) could be consolidated into a single nested rule to reduce duplication and improve maintainability.♻️ Proposed consolidation
-// Gridstack adds .ui-draggable-dragging to the <gridstack-item> wrapping this -// card while a drag is active. Override the open-palm `grab` cursor with the -// closed-fist `grabbing` cursor for the duration of the drag. -:host-context(.ui-draggable-dragging) .component-card--editing { - cursor: grabbing !important; -} - -// Keep the edit-mode highlight (blue border + resize-corner icon) visible on -// the dragged card. The browser stops firing :hover events during a drag, so -// we re-apply the same styles using the same Gridstack hook used for cursor. -:host-context(.ui-draggable-dragging) .component-card--editing { - --sapTile_BorderColor: var(--sapHighlightColor, `#0070f2`); - --sapGroup_ContentBorderColor: var(--sapHighlightColor, `#0070f2`); -} - -:host-context(.ui-draggable-dragging) .component-card--editing .card__resize-indicator { - opacity: 1; -} +// Gridstack adds .ui-draggable-dragging to the <gridstack-item> wrapping this +// card while a drag is active. Override the open-palm `grab` cursor with the +// closed-fist `grabbing` cursor, and keep the edit-mode highlight (blue border +// + resize-corner icon) visible (the browser stops firing :hover during drag). +:host-context(.ui-draggable-dragging) .component-card--editing { + cursor: grabbing !important; + --sapTile_BorderColor: var(--sapHighlightColor, `#0070f2`); + --sapGroup_ContentBorderColor: var(--sapHighlightColor, `#0070f2`); + + .card__resize-indicator { + opacity: 1; + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/ngx/declarative-ui/dashboard/card/dashboard-card.component.scss` around lines 51 - 68, Multiple CSS blocks target the same host-context drag state; consolidate by combining the three :host-context(.ui-draggable-dragging) rule blocks into a single rule that nests all selectors for .component-card--editing and .component-card--editing .card__resize-indicator so the cursor: grabbing, the custom properties (--sapTile_BorderColor, --sapGroup_ContentBorderColor) and the opacity: 1 for .card__resize-indicator are applied together under one :host-context(.ui-draggable-dragging) selector to remove duplication and improve maintainability.projects/ngx/declarative-ui/dashboard/models/_breakpoints.scss (1)
12-18: Breakpoints in _breakpoints.scss align with DASHBOARD_BREAKPOINTS (TypeScript).
- SCSS sets columns at max-widths 599→4, 1023→8, 1439→12, and uses the default 14 columns above 1439—matching the TS entries (including the { w: 4000, c: 14 } ceiling for Gridstack).
- Line 3 is a standalone
//spacer; ifscss/comment-no-empty(or similar) is enforced, replace it with a non-empty comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/ngx/declarative-ui/dashboard/models/_breakpoints.scss` around lines 12 - 18, The SCSS variable $dashboard-breakpoints contains a standalone "//" empty spacer comment which violates scss/comment-no-empty; update the spacer to a non-empty comment (e.g., explain the default 14-column behavior or reference DASHBOARD_BREAKPOINTS) so the comment is not empty, keeping the existing breakpoint tuples ((599, 4), (1023, 8), (1439, 12)) and the explanatory lines about the default 14 columns and 4000 ceiling intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/ngx/declarative-ui/dashboard/card/dashboard-card.component.scss`:
- Line 77: Add an empty line immediately before the inline SCSS comment "//
Anchored to the .component-card edges. The card has horizontal padding" in
dashboard-card.component.scss to satisfy the
scss/double-slash-comment-empty-line-before Stylelint rule; simply insert a
blank line above that comment so the linter no longer flags the double-slash
comment.
In `@projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.scss`:
- Around line 19-26: Insert a single blank line immediately before the
double-slash comment that begins "// Acts as the inline-size query container..."
in the dashboard component SCSS so the comment is separated from the preceding
declaration (the line with padding). Leave the surrounding rules and properties
(container-type: inline-size; container-name: mfp-dashboard; and any references
to .mfp-sections-container) unchanged; this satisfies the
scss/double-slash-comment-empty-line-before lint rule.
In `@projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.ts`:
- Around line 6-7: Merge the two duplicate imports from '../models' into a
single import statement: import CardConfig, DashboardConfig, SectionConfig and
DASHBOARD_BREAKPOINTS together from '../models' (i.e., replace the separate
imports for CardConfig/DashboardConfig/SectionConfig and DASHBOARD_BREAKPOINTS
with one consolidated import that includes all four symbols) to resolve the
no-duplicate-imports lint error.
In `@projects/ngx/declarative-ui/dashboard/models/breakpoints.ts`:
- Around line 30-32: Replace the explicit ReadonlyArray<> form with the readonly
T[] array notation for DASHBOARD_BREAKPOINTS: update the type of the exported
DASHBOARD_BREAKPOINTS constant (the declaration using
ReadonlyArray<Readonly<Required<Pick<Breakpoint, 'w' | 'c'>> & { layout:
LayoutStrategy }>>) to use the readonly array shorthand (readonly ...[]) so it
satisfies the `@typescript-eslint/array-type` rule while keeping the inner
Readonly/Required/Pick composition intact.
---
Outside diff comments:
In `@projects/ngx/declarative-ui/stories/dashboard.stories.ts`:
- Around line 48-60: The table-pods card config is duplicated between
AVAILABLE_CARDS and the CARDS array; extract the shared object into a factory or
constant (e.g., createTablePodsCard or TABLE_PODS_CONFIG) that returns the base
CardConfig for id 'table-pods' (including w, h, component, componentInputs with
TABLE_CARD_CONFIG/TABLE_RESOURCES and header/headerTooltip), then replace the
inline definition in AVAILABLE_CARDS with a call to that factory and pass {
label: 'Pods Table' } as an override where the label is only needed; update
CARDS to use the same factory without the label to remove duplication.
---
Nitpick comments:
In `@projects/ngx/declarative-ui/dashboard/card/dashboard-card.component.scss`:
- Around line 51-68: Multiple CSS blocks target the same host-context drag
state; consolidate by combining the three :host-context(.ui-draggable-dragging)
rule blocks into a single rule that nests all selectors for
.component-card--editing and .component-card--editing .card__resize-indicator so
the cursor: grabbing, the custom properties (--sapTile_BorderColor,
--sapGroup_ContentBorderColor) and the opacity: 1 for .card__resize-indicator
are applied together under one :host-context(.ui-draggable-dragging) selector to
remove duplication and improve maintainability.
In `@projects/ngx/declarative-ui/dashboard/models/_breakpoints.scss`:
- Around line 12-18: The SCSS variable $dashboard-breakpoints contains a
standalone "//" empty spacer comment which violates scss/comment-no-empty;
update the spacer to a non-empty comment (e.g., explain the default 14-column
behavior or reference DASHBOARD_BREAKPOINTS) so the comment is not empty,
keeping the existing breakpoint tuples ((599, 4), (1023, 8), (1439, 12)) and the
explanatory lines about the default 14 columns and 4000 ceiling intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 395d6974-fc08-413a-a838-78476b4388b2
📒 Files selected for processing (13)
projects/ngx/declarative-ui/dashboard/card/dashboard-card.component.htmlprojects/ngx/declarative-ui/dashboard/card/dashboard-card.component.scssprojects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.htmlprojects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.scssprojects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.tsprojects/ngx/declarative-ui/dashboard/models/_breakpoints.scssprojects/ngx/declarative-ui/dashboard/models/breakpoints.tsprojects/ngx/declarative-ui/dashboard/models/index.tsprojects/ngx/declarative-ui/dashboard/section/dashboard-section.component.htmlprojects/ngx/declarative-ui/dashboard/section/dashboard-section.component.scssprojects/ngx/declarative-ui/dashboard/section/dashboard-section.component.tsprojects/ngx/declarative-ui/stories/dashboard.cards.tsprojects/ngx/declarative-ui/stories/dashboard.stories.ts
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Closed
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
Improvements
Style