-
Notifications
You must be signed in to change notification settings - Fork 294
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
chore: unify rendering of Recover Banner #6425
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 5 Ignored Deployments
|
Is it expected that we don't have the purple background in the recover card now ? |
Can we make sure with @Anthony-Ledger that we want to hide the action cards and the portfolio carousel when the recover banner is displayed ? |
@cgrellard-ledger it's what the spec says. "Recover takes the lead (depends what’s easier technically)" |
Yep let's discuss this, I'm not sure how to understand the "takes the lead". It will an impact on the growth team if we do it this way. I'll get Anthony's input just to confirm |
Anthony is fine with this solution ✅ |
There as been no activity on this PR for the last 14 days. Please consider closing this PR. |
fixes LIVE-11239
77595fe
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.
LGTM just a small suggestion if you want to use it
/** | ||
* @prop children: if a child is passed, it will be rendered instead of the default banner. this allows to do a passthrough to have first the recover banner, then the rest of the content. | ||
*/ | ||
export default function RecoverBanner({ children }: { children?: React.ReactNode }) { |
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.
You can use this type too if you want
export default function RecoverBanner({ children }: { children?: React.ReactNode }) { | |
export default function RecoverBanner({ children }: React.PropsWithChildren) { |
…action card was received but its feature flag was disabled
📝 Description
This makes sure that only one banner at a time is displayed, before this fix, the recover banner could appear on top of the content card banner, instead we have made it so the recover banner is first displayed before the second. This is implemented with a "passthrough" pattern (the RecoverBanner will render any children that is passed if it doesn't need to render anything, that way RecoverBanner remains decoupled from ContentCards)
On top of this, we have fixed inconsistencies in the UI of the Recover Banner that was fixed on this PR by making RecoverBanner renders an ActionCard. a leftContent generic prop was introduced to allow to inject custom left content instead of an image.
out.mov
❓ Context
✅ Checklist
Pull Requests must pass the CI and be code reviewed. Set as Draft if the PR is not ready.
npx changeset
was attached.🧐 Checklist for the PR Reviewers