Fixed: Flash / alert banner multiple line text wrap around icon

[amarmanhala]

Describe your changes here.

Closes #3385

Screenshots

Before

After

Merge checklist

• Added/updated tests
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

[changeset-bot]

🦋 Changeset detected

Latest commit: 79d10d4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

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

[amarmanhala]

Hello, @joshblack @broccolinisoup guys, I'm still relatively new to the codebase. Could you please review my pull request and let me know if there are any areas that need improvement? Your feedback would be greatly appreciated!

[broccolinisoup]

Hi @amarmanhala 👋🏻 Thanks for your contribution!

There might be an issue going on in our end regarding the deployment of fork pull requests' branches. I cannot see your changes on the storybook https://primer-c9fd71df69-13348165.drafts.github.io/storybook/ which is strange - I'll debug the issue and check with my team and get back to you. Thanks for your patience 🙏🏻

[amarmanhala]

Hi @amarmanhala 👋🏻 Thanks for your contribution!

There might be an issue going on in our end regarding the deployment of fork pull requests' branches. I cannot see your changes on the storybook https://primer-c9fd71df69-13348165.drafts.github.io/storybook/ which is strange - I'll debug the issue and check with my team and get back to you. Thanks for your patience 🙏🏻

Hi, @broccolinisoup. Okay, no worries. I hope the deployment issue is fixed soon. Thank you for updating me.

[amarmanhala]

Hi @amarmanhala 👋🏻 Thanks for your contribution!
There might be an issue going on in our end regarding the deployment of fork pull requests' branches. I cannot see your changes on the storybook https://primer-c9fd71df69-13348165.drafts.github.io/storybook/ which is strange - I'll debug the issue and check with my team and get back to you. Thanks for your patience 🙏🏻

Hi, @broccolinisoup. Okay, no worries. I hope the deployment issue is fixed soon. Thank you for updating me.

Hi, @broccolinisoup. May I know if there is any update on this issue? Or do I need to push/update any changes from my end? Thank you

[broccolinisoup]

Hi @amarmanhala 👋🏻 I am so sorry for the delay! Our current automated workflow for deploying fork changes to storybook has some issues but I went ahead and fetched your changes on my local to review your PR. Thanks so much for pushing this PR and also your patience with me 🙏🏻

I think this issue is trickier than I initially thought. Your changes look great to me for the storybook examples however there are a few use cases I thought of that the changes in this PR can consider possibly breaking.

1. Flash component that has multiple elements as children

 <Flash className="mb-3">
    <Octicon icon={CheckIcon} />
    <span>
      Lorem Ipsum dolor sit amet Lorem Ipsum dolor sit amer Lorem Ipsum dolor sit amer Lorem Ipsum dolor sit amet {' '}
    </span>
      Lorem Ipsum dolor sit amet Lorem Ipsum dolor sit amer Lorem Ipsum dolor sit amer Lorem Ipsum dolor sit amet {' **'}**
  </Flash>

Because we add display:flex to the wrapper div in this PR, the layout of the children elements is changed.

Before

After

2. Another thing to note that because Flash component doesn't restrict the type of the children passed into, it could be anything. For example the example below renders the children text as p and p element has its default margin top and our margin-top:${get('space.1')}; won't align the icon with the first line of text.

<Flash>
  <CheckIcon />
  <Text as="p">
    Lorem ipsum dolor sit amet lorem ipsum
    <Text sx={{fontWeight: 'bold'}}>dolor sit amet</Text> lorem ipsum.
  </Text>
</Flash>

All that said, I am unsure what to recommend here. It feels to me that the styling will very much depend on the use case. I'll chat about it with my team to see what they think but in the meantime please let me know if you have any questions or concerns about my comment. If you have a solution to propose to address the concerns I mentioned, you are more than welcome to push a commit. We appreciate your contribution so much 🙏🏻

[langermank]

I think this issue is trickier than I initially thought

I agree with @broccolinisoup here. This component needs a more substantial refactor which would include making the icon part of the API instead of expecting consumers to pass one in. We have a standard set of icons for each state, and this is how we handle it in other Primer implementations. That way, we can have more control over the layout of icon and children.

I wouldn't recommend moving forward with these changes at this time as it will likely lead to visual regressions.

[amarmanhala]

Hi @amarmanhala 👋🏻 I am so sorry for the delay! Our current automated workflow for deploying fork changes to storybook has some issues but I went ahead and fetched your changes on my local to review your PR. Thanks so much for pushing this PR and also your patience with me 🙏🏻

I think this issue is trickier than I initially thought. Your changes look great to me for the storybook examples however there are a few use cases I thought of that the changes in this PR can consider possibly breaking.

1. Flash component that has multiple elements as children

 <Flash className="mb-3">
    <Octicon icon={CheckIcon} />
    <span>
      Lorem Ipsum dolor sit amet Lorem Ipsum dolor sit amer Lorem Ipsum dolor sit amer Lorem Ipsum dolor sit amet {' '}
    </span>
      Lorem Ipsum dolor sit amet Lorem Ipsum dolor sit amer Lorem Ipsum dolor sit amer Lorem Ipsum dolor sit amet {' **'}**
  </Flash>

Because we add display:flex to the wrapper div in this PR, the layout of the children elements is changed.

Before

**After** 2. Another thing to note that because Flash component doesn't restrict the type of the children passed into, it could be anything. For example the example below renders the children text as `p` and `p` element has its default margin top and our `margin-top:${get('space.1')};` won't align the icon with the first line of text.

<Flash>
  <CheckIcon />
  <Text as="p">
    Lorem ipsum dolor sit amet lorem ipsum
    <Text sx={{fontWeight: 'bold'}}>dolor sit amet</Text> lorem ipsum.
  </Text>
</Flash>

All that said, I am unsure what to recommend here. It feels to me that the styling will very much depend on the use case. I'll chat about it with my team to see what they think but in the meantime please let me know if you have any questions or concerns about my comment. If you have a solution to propose to address the concerns I mentioned, you are more than welcome to push a commit. We appreciate your contribution so much 🙏🏻

Hi @broccolinisoup, Yes, you are right this issue is trickier to fix at this point. I totally missed testing how my changes affected this component with respect to their children.

But yeah Thank you for your feedback and for taking the time to review the pull request. I understand the concerns you've raised, especially regarding potential layout changes for components with various children.

I think making changes to the core of a component can be challenging and may require careful consideration. It's important to ensure that any changes maintain compatibility with existing use cases while providing enhancements for new use cases.

I will figure out if I can provide any solution.

Thank you

[github-actions]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.