-
Notifications
You must be signed in to change notification settings - Fork 3
Added feedbackMessage & hasError paramaters to OSS::Banner #632
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
base: master
Are you sure you want to change the base?
Conversation
| @@ -1,4 +1,6 @@ | |||
| .upf-banner { | |||
| position: relative; | |||
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.
Same kind of comment as on the initial PR, this might have a negative impact on existing usages.
I'm guessing it was required because in the template, you included the upf-banner--feedback within the scope of the "banner" div ?
How about putting it outside ?
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.
Come to think of it, I'm unsure we can go around this and that's probably why you added it in ^^
Using relative here, can you check how everything is working in upfluence-web ? 🙏
I think Olympe had found a spot which was breaking to the initial changes from her PR, check if it's the case with this new implementation and also with other banners?
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.
if it does end up messing things up, I have a final idea which would be to use a pseudo-element and inserting the feedback message into the content attribute
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.
The spot where it was breaking was in workflow > campaign > select an influencer > export selection 😉
| plain?: boolean; | ||
| selected?: boolean; | ||
| disabled?: boolean; | ||
| hasError?: boolean; |
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.
Open question, why using a hasError now? The feedbackMessage can take a undefined for value
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.
Looks like in the current typing
oss-components/addon/components/o-s-s/input-container.ts
Lines 5 to 8 in c610db1
| export type FeedbackMessage = { | |
| type: 'error' | 'warning' | 'success'; | |
| value: string; | |
| }; |
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.
The project requires being able to display a red border with AND without having a feedback message under (see here).
As Antoine pointed out, the feedbackMessage value isn't defined as being optional, so I added the @hasError parameter we've used on other OSS components to tackled this need 👍
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.
We did a change in the feature/payment-requests-onboarding-rework branch to set value undefined and more this types all a "global" one
| plain?: boolean; | ||
| selected?: boolean; | ||
| disabled?: boolean; | ||
| hasError?: boolean; |
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.
Looks like in the current typing
oss-components/addon/components/o-s-s/input-container.ts
Lines 5 to 8 in c610db1
| export type FeedbackMessage = { | |
| type: 'error' | 'warning' | 'success'; | |
| value: string; | |
| }; |
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
What does this PR do?
Related to: #vel-7099
This PR aims at adding the option to handle different states on the
OSS::Bannercomponent.The
feedbackMessageparameter includes atype(error || warning || success) and avalue.It lets us display a message under the banner, based off of its
value, while itstypechanges the font color of the message & the border color of the banner accordingly.The
hasErrorparameter lets us display a red border on the banner without the need to pass a feedback message.What are the observable changes?
Good PR checklist
Note
Low Risk
Scoped to UI component styling/templating with added tests; main risk is visual/layout regressions due to new absolute-positioned feedback message and margin changes.
Overview
OSS::Bannernow supports two new inputs:@hasErrorto force an error border, and@feedbackMessage({type: 'error' | 'warning' | 'success', value}) to show a message below the banner and apply matching border + text colors.This adds validation/shared
FEEDBACK_TYPES, new banner border modifier classes (upf-banner--error|warning|success), and layout tweaks (relative positioning + extra bottom margin/absolute-positioned feedback text). Storybook, dummy app examples, and integration tests were updated to cover the new states and invalid feedback types.Written by Cursor Bugbot for commit b881523. This will update automatically on new commits. Configure here.