-
Notifications
You must be signed in to change notification settings - Fork 478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fleet UI: Info banner component, uses Card as base component, tests #26276
base: main
Are you sure you want to change the base?
Conversation
pageLevel?: boolean; | ||
/** Add this element to the end of the banner message. Mutually exclusive with `link`. */ | ||
cta?: JSX.Element; | ||
/** closable and link are mutually exclusive */ | ||
closable?: boolean; | ||
/** Makes the entire banner clickable */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing through instead since path
is prop on Card
/** default 4px */ | ||
borderRadius?: "large" | "xlarge"; | ||
borderRadius?: "medium" | "xlarge"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corresponds to Card border radius sizes
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #26276 +/- ##
=======================================
Coverage 63.66% 63.66%
=======================================
Files 1631 1631
Lines 156545 156533 -12
Branches 4046 4037 -9
=======================================
- Hits 99657 99656 -1
+ Misses 49038 49026 -12
- Partials 7850 7851 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -57,7 +57,7 @@ const MainContent = ({ | |||
banner = <AppleBMTermsMessage />; | |||
} else if (isVppExpired || willVppExpire) { | |||
banner = <VppRenewalMessage expired={isVppExpired} />; | |||
} else if (isFleetLicenseExpired) { | |||
} else if (!isFleetLicenseExpired) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Change this back - used for testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had a quick question about the info banner path behaviour
className={wrapperClasses} | ||
color={color} | ||
borderRadiusSize={borderRadius} | ||
path={path} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that Info Banner itself is clickable and will go to a path? Is this the correct behaviour? seems strange as it looks like the clickable area in the design is a link
,action
, or x
button. Im also not seeing where we are using this prop in the user of <InfoBanner />
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghernandez345
link
was the former prop that had been used before, but no longer is used anywhere. I think whatever used to use link
was got moved to use a <Card>
directly and changed to path
. I replaced it with path
prop since that was the corresponding prop for the basecomponent <Card>
but I can see how that is confusing because we have cta
which is also clickable.
Issue
Part 2 for #26231
Description
Screenshots
TODO: After confirmation from Design
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/
,orbit/changes/
oree/fleetd-chrome/changes
.