feat: Alert/Aside component #152
Conversation
🦋 Changeset detectedLatest commit: 8c6f410 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds a new Alert UI component to the studio CMS UI package with typed props (title, variant, icon), seven visual variants, CSS theming, exports, documentation, a test fixture, Vitest unit tests, and Playwright end-to-end tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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 |
Adammatthiesen
left a comment
Adammatthiesen
left a comment
There was a problem hiding this comment.
Biggest thing... make sure your linting with the repo's linter... and our linter only...
|
Oh i just noticed this was a draft.... 😅 |
- also updated `colors.css` to pass accessiblity tests.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Okay, here is my current checklist...
- vitest snapshots are currently failing (you will need to delete the old snapshots and regenerate them)
-
- There is still CSS marked "Only for testing" what is this? can we remove it?
- Needs a changeset (run
pnpm changeset) and create a minor changeset for the new component
Not sure if Lou has anything else to add
|
@Adammatthiesen cleaned up, with changesets. |
jellydeck
left a comment
jellydeck
left a comment
There was a problem hiding this comment.
requested changes applied
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/studiocms_ui/test/components/Alert.e2e.ts`:
- Around line 44-76: The tests "Renders alert content correctly" and "Renders
with correct variant icons" are missing assertions for the primary variant;
update both tests to include checks for the primary variant by using the same
variantSection locator and selectors: add an assertion that
variantSection.locator('[data-variant="primary"] .sui-alert-content') contains
the primary alert text (e.g., 'This is a primary alert message.') in the content
test, and add an assertion that variantSection.locator('[data-variant="primary"]
.sui-alert-icon') is visible in the icon visibility test so the primary variant
is covered like the other variants.
In `@packages/studiocms_ui/test/components/Alert.test.ts`:
- Around line 14-23: The variants array in the test "renders Alert component
with different variants" is missing the supported "primary" variant; update the
variants constant used in that test (the local variable named variants) to
include "primary" alongside 'default', 'success', 'danger', 'info', 'warning',
and 'mono' so the Alert component (Alert) test covers all StudioCMSColorway
options.
🧹 Nitpick comments (3)
packages/studiocms_ui/src/components/Alert/alert.css (1)
31-71: Consider scoping the variant selectors to.sui-alert.The
[data-variant="..."]selectors are not scoped to.sui-alert, which could unintentionally style other components that use the samedata-variantattribute pattern.♻️ Suggested refactor
-.sui-alert-title { - font-weight: 600; - font-size: 1.125em; -} - -[data-variant="default"] { +.sui-alert[data-variant="default"] { --alert-accent: var(--text-normal); color: var(--text-dimmed); background-color: var(--default-base); } -[data-variant="primary"] { +.sui-alert[data-variant="primary"] { --alert-accent: var(--primary-base); color: var(--text-dimmed); background-color: var(--primary-flat); } -[data-variant="success"] { +.sui-alert[data-variant="success"] { --alert-accent: var(--success-base); color: var(--text-dimmed); background-color: var(--success-flat); } -[data-variant="warning"] { +.sui-alert[data-variant="warning"] { --alert-accent: var(--warning-base); color: var(--text-dimmed); background-color: var(--warning-flat); } -[data-variant="danger"] { +.sui-alert[data-variant="danger"] { --alert-accent: var(--danger-vibrant); color: var(--text-dimmed); background-color: var(--danger-flat); } -[data-variant="info"] { +.sui-alert[data-variant="info"] { --alert-accent: var(--info-vibrant); color: var(--text-dimmed); background-color: var(--info-flat); } -[data-variant="mono"] { +.sui-alert[data-variant="mono"] { --alert-accent: var(--mono-base); color: var(--text-dimmed); background-color: var(--mono-flat); }packages/studiocms_ui/src/components/Alert/Alert.astro (1)
39-39: Simplify the class conditional.The
className && classNamepattern is redundant. Astro'sclass:listalready handles falsy values gracefully.♻️ Suggested fix
-<div class:list={["sui-alert", className && className]} data-variant={variant} {...props}> +<div class:list={["sui-alert", className]} data-variant={variant} {...props}>packages/studiocms_ui/test/components/Alert.test.ts (1)
57-66: Consider extracting the variants array to reduce duplication.The
variantsarray is defined identically on lines 15 and 58. Extracting it to a module-level constant would improve maintainability—if a new variant is added, you'd only need to update one place.♻️ Suggested refactor
import { describe, expect } from 'vitest'; import Alert from '../../src/components/Alert/Alert.astro'; import { test } from '../fixtures/vitest/AstroContainer'; +const ALERT_VARIANTS = ['default', 'success', 'danger', 'info', 'warning', 'mono'] as const; + describe('Alert Component', () => { test('renders Alert component with different variants', async ({ renderComponent }) => { - const variants = ['default', 'success', 'danger', 'info', 'warning', 'mono']; - for (const variant of variants) { + for (const variant of ALERT_VARIANTS) { // ... } }); // ... later in the file ... test('renders Alert with correct data-variant attribute', async ({ renderComponent }) => { - const variants = ['default', 'success', 'danger', 'info', 'warning', 'mono']; - for (const variant of variants) { + for (const variant of ALERT_VARIANTS) { // ... } }); });
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Adammatthiesen
left a comment
Adammatthiesen
left a comment
There was a problem hiding this comment.
That clears up the things I saw. Will leave it to the UI team now
RATIU5
left a comment
RATIU5
left a comment
There was a problem hiding this comment.
@jellydeck If you're able to, create a docs page that goes over the usage and features of this component. Feel free to use another page as a reference found in docs/src/content/docs/docs/components.
jellydeck
dismissed stale reviews from Adammatthiesen and coderabbitai[bot]
via
831c43f
@RATIU5 took other pages as reference and added docs. |
dreyfus92
left a comment
dreyfus92
left a comment
There was a problem hiding this comment.
Awesome job @jellydeck, this looks good to me. 😄
Adammatthiesen
left a comment
Adammatthiesen
left a comment
There was a problem hiding this comment.
Snapshots are outdated again due to change.
jellydeck
dismissed stale reviews from dreyfus92 and coderabbitai[bot]
via
75f4cdc
Adammatthiesen
left a comment
Adammatthiesen
left a comment
There was a problem hiding this comment.
Hey this is looking like the next new StudioCMS UI Component! 🎉 Great work and thanks for bearing with our nitpicks!
@Adammatthiesen appreciate it :) |
5 participants
Closes #132
Description
default | success | danger | info | warning | monoSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.