Add fillWidth to Alert prop options #668
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a fillWidth prop to the Alert component that allows the alert content to expand to the full available width. This is useful for scenarios where content (like right-aligned buttons) needs the full width of the alert container.
- Adds
fillWidthboolean prop with default value offalse - Updates the
TextWrapperstyled component to conditionally apply 100% width - Refactors inline styled component type definitions to proper interfaces
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| display: flex; | ||
| flex-flow: column; | ||
| word-break: break-word; | ||
| ${({ $fillWidth }) => $fillWidth && "width: 100%;"}; |
There was a problem hiding this comment.
[nitpick] The conditional CSS string should use a template literal or return a proper CSS rule. Consider using $fillWidth && css\width: 100%;`` or wrapping in a proper CSS block for better readability and consistency with styled-components patterns.
Copilot uses AI. Check for mistakes.
| display: flex; | ||
| flex-flow: column; | ||
| word-break: break-word; | ||
| ${({ $fillWidth }) => $fillWidth && "width: 100%;"}; |
There was a problem hiding this comment.
- I'd suggest using
flex-grow: 1instead - Why do we need it to be conditional? What is the use case for text content to not be full-width?
And I'm asking because I'd be fine with it being a prop, but other components that expect fillWidth apply it to the root container. So I would expect this behavior from every component. But in this case it applies to an internal element. This brings inconsistency.
My suggestion is to just apply flex-grow: 1 unconditionally and make it the default behavior. Unless we know of a use case where we do need it to be not full-width. In which case I suggest coming up with a different prop name to avoid confusion.
|
Closing this after a discussion with design team as it is unnecessary |
Most of the time the alert content is having the width of auto which is ok
But if the user wants a content that needs to extend to get the whole space this would fail and thats why fillWidth is introduced
Example: Adding a list of button to be aligned to the end of the alert (right aligned)