primer / react

An implementation of GitHub's Primer Design System using React
https://primer.style/guides/react
MIT License
3.18k stars 538 forks source link

Flash / alert banner text wrap around icon #3385

Closed lukasoppermann closed 5 months ago

lukasoppermann commented 1 year ago

Description

If a flash banner has text that flows on multiple lines it wraps around the icon.

Issue shown in react docs

Expected

Text should wrap within a block and icon should be in front.

Screenshot of expected result

Steps to reproduce

  1. Go to https://primer.style/react/Flash#with-an-icon
  2. Edit the flash alert to include more text so that it wraps onto a second line

Version

v35.25.1

Browser

Chrome

Shivam-Amin commented 1 year ago

Hey there! Can I work on this issue?

lesliecdubs commented 1 year ago

@Shivam-Amin yes, we would welcome the contribution! Let us know if you have questions.

I have to separate icon & text at some gap, as shown in expected section. Yes, the change requested is for all subsequent lines of text to be indented. You may use the Banner implementation from the sister Primer ViewComponents library as an example: https://view-components-storybook.eastus.cloudapp.azure.com/view-components/lookbook/inspect/primer/alpha/banner/playground

Shivam-Amin commented 1 year ago

I'm not able to run npm run setup & I'm getting this error. How do I solve it?

img containing the error I'm getting while running ```npm run setup```

github-actions[bot] commented 1 year ago

Uh oh! @Shivam-Amin, the image you shared is missing helpful alt text. Check https://github.com/primer/react/issues/3385#issuecomment-1600785400.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

lesliecdubs commented 1 year ago

@Shivam-Amin can you confirm that you are running Node.js v16? You can check your node version with node -v. If you're not on 16, you can manage the version installed with nvm.

If you haven't already checked these out, here's a link to the contributor docs which may be helpful.

Shivam-Amin commented 1 year ago

@lesliecdubs, still I'm facing the same issue after switching to Node.js v16. & Do I have to install storybook in order to run the site locally?

Regarding the error I'm facing at the time of running npm run setup

lesliecdubs commented 1 year ago

I'm going to tag in one of our first responders to see if they can help troubleshoot.

josepmartins commented 1 year ago

hey @Shivam-Amin 👋 !

can you try doing the setup manually? this is the script https://github.com/primer/react/blob/main/script/setup, basically you'll need to:

cc @joshblack in case there's something I'm missing 😅

Shivam-Amin commented 1 year ago

I still facing the same error while running npm run build.

amareshvarma commented 1 year ago

@lesliecdubs @josepmartins I have done exactly what you have mentioned above Primer-error

I am facing the above issue and not able to run( I am using node 18)

Shivam-Amin commented 1 year ago

@amareshvarma Use node version 16.

amareshvarma commented 1 year ago

@Shivam-Amin were you able to run successfully?

Shivam-Amin commented 1 year ago

Not yet. But trying to solve it.

amarmanhala commented 1 year ago

Hi @lesliecdubs Can I fix this issue?

Shivam-Amin commented 1 year ago

Hi @amarmanhala, As I'm not able to set up the project locally, I'm not able to solve this issue.

lesliecdubs commented 1 year ago

Reassigning to you, @amarmanhala!

amarmanhala commented 1 year ago

Thank you @lesliecdubs

amarmanhala commented 1 year ago

Hi @lesliecdubs, As I was working on this issue, I need your thought on it. Which icon placement option do you think is more intuitive and user-friendly? 1. Icon at the Top (screenshot 1) or Icon in the center? I liked the 2 options. 1

flash2

2

flash1
lesliecdubs commented 1 year ago

Hi @amarmanhala 👋🏻 Ideally, we would prefer the icon to be aligned horizontally with the first line of text, as shown in the "Expected" section of the issue summary:

Image of a blue banner with rounded corners, with an i information icon beside title and message text
amarmanhala commented 1 year ago

Okay got it now, Thank you @lesliecdubs for your explanation!

broccolinisoup commented 11 months ago

I believe this issue requires a breaking change please see https://github.com/primer/react/pull/3634#pullrequestreview-1647835818 and https://github.com/primer/react/pull/3634#issuecomment-1739930260

github-actions[bot] commented 5 months ago

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

lukasoppermann commented 5 months ago

This should be changed in the new banner implementation.