Closed
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
3 similar comments
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
📝 WalkthroughWalkthroughStorybook stories for Alert and Modal components were refactored to use args-driven rendering patterns, with new demo variations and stories added. Component padding and modal border styling were adjusted, with corresponding CSS overrides applied to the plugin UI root. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
1 similar comment
📝 WalkthroughWalkthroughStorybook stories for Alert and Modal components were refactored to use args-driven rendering patterns, with new demo variations and stories added. Component padding and modal border styling were adjusted, with corresponding CSS overrides applied to the plugin UI root. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughStorybook stories for Alert and Modal components were refactored to use args-driven rendering patterns, with new demo variations and stories added. Component padding and modal border styling were adjusted, with corresponding CSS overrides applied to the plugin UI root. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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
🤖 Fix all issues with AI agents
In `@src/components/ui/Alert.stories.tsx`:
- Around line 130-149: In the WithButtons Story (render function), the
col-start-2 utility on the inner div is ineffective because AlertIcon is nested
inside that flex wrapper instead of being a direct child of Alert; either remove
the dead class "col-start-2" if you intend a single-row horizontal layout, or
move the <AlertIcon> component out of the inner <div className="flex..."> so
AlertIcon becomes a direct child of Alert (which will trigger the
grid-cols-[auto_1fr] two-column layout); update the WithButtons story
accordingly.
In `@src/components/ui/Modal.stories.tsx`:
- Line 96: The FieldDescription for the Countries field is using the wrong
constant (ZONE_NAME_HELPER); change it to the appropriate countries helper
(e.g., COUNTRIES_HELPER or a new constant like COUNTRIES_HELPER = "Select one or
more countries for this zone") and update the usage in the Modal story where
FieldDescription currently renders ZONE_NAME_HELPER; also ensure the helper
constant is imported or defined alongside other helper constants so the
Countries field shows the correct descriptive text.
In `@src/components/ui/modal.tsx`:
- Around line 83-87: The modal header/footer lost the Tailwind border utility so
borders no longer follow the theme token; restore the theme-driven border by
re-adding the Tailwind class (e.g., "border-border") to the className used on
the element with data-slot="modal-header" and the ModalFooter element, and fix
the scoped CSS override (in styles.css) that currently hardcodes `#E9E9E9` by
changing it to use border-color: var(--border) so the design token and
ThemeProvider/dark mode continue to control the border color.
In `@src/styles.css`:
- Around line 249-259: The modal header/footer CSS uses a hardcoded color
`#E9E9E9` which bypasses design tokens and breaks dark mode; update the selectors
.pui-root [data-slot="modal-header"] and .pui-root [data-slot="modal-footer"] to
use the design token (e.g. border: 1px solid var(--border)) instead of the hex
value, or remove these overrides entirely if the Tailwind class (border-border)
is restored in modal.tsx so the tokenized border is applied consistently.
🧹 Nitpick comments (1)
src/components/ui/Modal.stories.tsx (1)
80-95: Dropdown trigger text is static — consider noting this is intentional for the demo.The trigger button always shows "Bangladesh, United States" regardless of any selection state. This is fine for a Storybook demo, but adding a brief comment would clarify it's intentionally static and not a bug.
Comment on lines
+130
to
+149
| export const WithButtons: Story = { | ||
| args: { variant: defaultVariant }, | ||
| render: (args) => ( | ||
| <Alert variant={args.variant}> | ||
| <div className="flex items-center justify-between gap-4 col-start-2"> | ||
| <AlertIcon variant={args.variant ?? defaultVariant} /> | ||
| <AlertTitle>Hold on I need at least a few minutes!</AlertTitle> | ||
| <div className="flex gap-2"> | ||
| <Button variant="default">Primary</Button> | ||
| <Button variant="outline">Button</Button> | ||
| </div> | ||
| </div> | ||
| <AlertAction> | ||
| <Button variant="ghost" size="icon-sm" aria-label="Dismiss"> | ||
| <X /> | ||
| </Button> | ||
| </AlertAction> | ||
| </Alert> | ||
| ), | ||
| }; |
There was a problem hiding this comment.
col-start-2 has no effect in the WithButtons story.
The Alert grid only switches to a 2-column layout (grid-cols-[auto_1fr]) when it has-[>svg] as a direct child. In this story, the SVG (from AlertIcon) is nested inside the <div className="flex ..."> wrapper, so the grid stays single-column and col-start-2 on line 134 is dead CSS.
If the intent is a single-row horizontal layout, the class can simply be removed. If you want the 2-column grid to activate, move <AlertIcon> out of the flex wrapper to be a direct child of <Alert>.
🤖 Prompt for AI Agents
In `@src/components/ui/Alert.stories.tsx` around lines 130 - 149, In the
WithButtons Story (render function), the col-start-2 utility on the inner div is
ineffective because AlertIcon is nested inside that flex wrapper instead of
being a direct child of Alert; either remove the dead class "col-start-2" if you
intend a single-row horizontal layout, or move the <AlertIcon> component out of
the inner <div className="flex..."> so AlertIcon becomes a direct child of Alert
(which will trigger the grid-cols-[auto_1fr] two-column layout); update the
WithButtons story accordingly.
| ))} | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| <FieldDescription>{ZONE_NAME_HELPER}</FieldDescription> |
There was a problem hiding this comment.
Copy-paste issue: wrong helper text for the Countries field.
ZONE_NAME_HELPER ("Give a meaningful name for your reference") is reused as the description for the Countries field. This is clearly meant for the Zone Name field only.
Proposed fix
- <FieldDescription>{ZONE_NAME_HELPER}</FieldDescription>
+ <FieldDescription>Select the countries for this shipping zone</FieldDescription>📝 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.
Suggested change
| <FieldDescription>{ZONE_NAME_HELPER}</FieldDescription> | |
| <FieldDescription>Select the countries for this shipping zone</FieldDescription> |
🤖 Prompt for AI Agents
In `@src/components/ui/Modal.stories.tsx` at line 96, The FieldDescription for the
Countries field is using the wrong constant (ZONE_NAME_HELPER); change it to the
appropriate countries helper (e.g., COUNTRIES_HELPER or a new constant like
COUNTRIES_HELPER = "Select one or more countries for this zone") and update the
usage in the Modal story where FieldDescription currently renders
ZONE_NAME_HELPER; also ensure the helper constant is imported or defined
alongside other helper constants so the Countries field shows the correct
descriptive text.
Comment on lines
+83
to
87
| data-slot="modal-header" | ||
| className={cn( | ||
| "flex flex-col space-y-1.5 text-center sm:text-left border-b border-border py-4 px-8", | ||
| "flex flex-col space-y-1.5 text-center sm:text-left border-b py-4 px-8", | ||
| className, | ||
| )} |
There was a problem hiding this comment.
Removing border-border breaks theme-driven border colors.
By dropping the border-border Tailwind utility, the header border color is no longer tied to the --border design token. Instead it relies on a hardcoded #E9E9E9 override in styles.css. This means dark mode (.pui-root.dark) and any per-plugin theme overrides via ThemeProvider will not affect these borders.
Consider keeping the Tailwind utility and fixing the root cause (likely a CSS reset stripping border-color). For example, the scoped override in styles.css could set border-color: var(--border) instead of a hardcoded hex value, preserving the design-token pipeline.
Proposed fix (modal.tsx)
- "flex flex-col space-y-1.5 text-center sm:text-left border-b py-4 px-8",
+ "flex flex-col space-y-1.5 text-center sm:text-left border-b border-border py-4 px-8",The same applies to ModalFooter at Line 153.
🤖 Prompt for AI Agents
In `@src/components/ui/modal.tsx` around lines 83 - 87, The modal header/footer
lost the Tailwind border utility so borders no longer follow the theme token;
restore the theme-driven border by re-adding the Tailwind class (e.g.,
"border-border") to the className used on the element with
data-slot="modal-header" and the ModalFooter element, and fix the scoped CSS
override (in styles.css) that currently hardcodes `#E9E9E9` by changing it to use
border-color: var(--border) so the design token and ThemeProvider/dark mode
continue to control the border color.
Comment on lines
+249
to
+259
| .pui-root [data-slot="modal-header"] { | ||
| border-bottom: 1px solid #E9E9E9; | ||
| } | ||
|
|
||
| /* ============================================ | ||
| Modal Footer (overrides reset border-color) | ||
| ============================================ */ | ||
|
|
||
| .pui-root [data-slot="modal-footer"] { | ||
| border-top: 1px solid #E9E9E9; | ||
| } |
There was a problem hiding this comment.
Hardcoded color #E9E9E9 bypasses the design-token system and breaks dark mode.
These overrides should use the existing --border token so that dark mode and per-plugin theming continue to work:
Proposed fix
.pui-root [data-slot="modal-header"] {
- border-bottom: 1px solid `#E9E9E9`;
+ border-bottom-color: var(--border);
}
.pui-root [data-slot="modal-footer"] {
- border-top: 1px solid `#E9E9E9`;
+ border-top-color: var(--border);
}If border-border is restored in the Tailwind classes (as suggested in modal.tsx), these CSS overrides may not be needed at all.
📝 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.
Suggested change
| .pui-root [data-slot="modal-header"] { | |
| border-bottom: 1px solid #E9E9E9; | |
| } | |
| /* ============================================ | |
| Modal Footer (overrides reset border-color) | |
| ============================================ */ | |
| .pui-root [data-slot="modal-footer"] { | |
| border-top: 1px solid #E9E9E9; | |
| } | |
| .pui-root [data-slot="modal-header"] { | |
| border-bottom-color: var(--border); | |
| } | |
| /* ============================================ | |
| Modal Footer (overrides reset border-color) | |
| ============================================ */ | |
| .pui-root [data-slot="modal-footer"] { | |
| border-top-color: var(--border); | |
| } |
🤖 Prompt for AI Agents
In `@src/styles.css` around lines 249 - 259, The modal header/footer CSS uses a
hardcoded color `#E9E9E9` which bypasses design tokens and breaks dark mode;
update the selectors .pui-root [data-slot="modal-header"] and .pui-root
[data-slot="modal-footer"] to use the design token (e.g. border: 1px solid
var(--border)) instead of the hex value, or remove these overrides entirely if
the Tailwind class (border-border) is restored in modal.tsx so the tokenized
border is applied consistently.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
3 similar comments
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Collaborator
Author
|
merged into this |
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.
add
Alertcomponents examples for the storybook