-
Notifications
You must be signed in to change notification settings - Fork 37
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
add trailingComponent prop to CTABanner component #587
Conversation
🦋 Changeset detectedLatest commit: c58eda8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
🟢 No design token changes found |
|
@@ -43,6 +43,15 @@ test.describe('Visual Comparison: CTABanner', () => { | |||
expect(await page.screenshot()).toMatchSnapshot() | |||
}) | |||
|
|||
test('CTABanner / With Trailing Component', async ({page}) => { |
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.
Can't see the new snapshot image in this PR. Could you try running it again please?
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.
Good spot 👍 Looks like they've generated now, although I'm not sure why Visual-Comparison-Tooltip-Tooltip-Label-Type-1-linux.png
has changed.
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.
Ah nvm, looks like they just weren't regenerated after this commit
015bfea#diff-7e60c94a63a2596dd7d646b8d1bbd5bc87baf66db760adc47f70150c93b842e5
Looks fine to me
@@ -68,6 +76,7 @@ const Root = forwardRef( | |||
> | |||
<div className={clsx(styles['CTABanner-content'], align === 'center' && styles['CTABanner-content--center'])}> | |||
{children} | |||
{TrailingComponent && <TrailingComponent />} |
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.
@joshfarrant How does this look with align="center"
? Could you share a screenie please?
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.
ofc, just added it to PR description
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.
This is looking great. Thanks @joshfarrant.
I can see visual tests are still being a bit flakey, so worth running one more time for luck before you merge this 😄 🤞
Summary
Add
trailingComponent
prop to CTABanner as an escape hatch to render arbitrary content.List of notable changes:
trailingComponent
propSupporting resources (related issues, external links, etc):
What should reviewers focus on?
Contributor checklist:
Reviewer checklist:
Screenshots: