fix: icon wrapper border on alert component #4383
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 6767a07 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
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 |
WalkthroughThis pull request addresses an issue with the NextUI Alert component's Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.changeset/many-kiwis-pretend.md (1)
6-6: Fix typo in changeset descriptionThere's a typo in the description: "borded" should be "border"
-Fix #4382 `hideIconWrapper` property wasn't removing the borded on `Alert` component +Fix #4382 `hideIconWrapper` property wasn't removing the border on `Alert` componentpackages/components/alert/src/use-alert.ts (1)
127-127: LGTM with a minor optimization suggestionThe logic correctly checks for both description and children content. However, we could optimize the isEmpty calls.
Consider caching the isEmpty results to avoid double computation:
-() => alert({hasContent: !isEmpty(description) || !isEmpty(children), ...variantProps}), +() => { + const hasDescription = !isEmpty(description); + const hasChildren = !isEmpty(children); + return alert({hasContent: hasDescription || hasChildren, ...variantProps}); +},packages/core/theme/src/components/alert.ts (1)
32-32: Improve icon centering with grid layoutThe switch from flex to grid layout with
place-items-centerprovides more reliable centering for the icon wrapper.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/many-kiwis-pretend.md(1 hunks)packages/components/alert/src/alert.tsx(1 hunks)packages/components/alert/src/use-alert.ts(1 hunks)packages/components/alert/stories/alert.stories.tsx(2 hunks)packages/core/theme/src/components/alert.ts(2 hunks)
🔇 Additional comments (5)
packages/components/alert/src/alert.tsx (1)
27-27: LGTM: Improved prop naming for better semantics
The change from hasDescription to hasContent in the AlertProps interface better represents the component's actual state, as it considers both description and children content.
packages/components/alert/stories/alert.stories.tsx (2)
229-235: LGTM! Good test coverage addition
The new WithoutIconWrapper story effectively demonstrates and tests the hideIconWrapper functionality, providing a clear example of the border fix implementation.
192-192: Verify spacing change impact
The removal of mt-3 class affects the vertical spacing of buttons. This change seems unrelated to the icon wrapper border fix.
✅ Verification successful
Spacing change appears to be intentional
The removal of mt-3 class aligns with the codebase's patterns. The search results show that the mt-3 class was previously used in the example code (apps/docs/content/components/alert/custom-styles.raw.tsx), but has been removed from both the example and the story for consistency. This change maintains visual harmony between the documentation and the component stories.
- The story now matches other similar button container implementations that use
flex items-center gap-1without additional margin classes - The change appears to be part of a deliberate styling update rather than an unrelated modification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of mt-3 class in alert stories
rg "mt-3" --type tsx
Length of output: 48
Script:
#!/bin/bash
# Search for mt-3 class in all files
rg "mt-3"
# Also check for similar margin classes that might be used for consistency
rg "className=\".*?flex items-center gap-1.*?\"" -A 2 -B 2
Length of output: 6569
packages/core/theme/src/components/alert.ts (2)
79-79: LGTM! Properly fixes icon wrapper border
The addition of !border-none along with !bg-transparent and !shadow-none effectively addresses the icon wrapper border issue when hideIconWrapper is true.
82-86: Verify impact of hasContent variant rename
The rename from hasDescription to hasContent and alignment updates seem to be a breaking change.
Closes #4382
📝 Description
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
hideIconWrapperproperty to correctly remove the border from the Alert component.