Skip to content
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

[Alert] html content breaks up text in Alert #20023

Closed
2 tasks done
simoami opened this issue Mar 8, 2020 · 9 comments · Fixed by #20490
Closed
2 tasks done

[Alert] html content breaks up text in Alert #20023

simoami opened this issue Mar 8, 2020 · 9 comments · Fixed by #20490
Labels
bug 🐛 Something doesn't work component: alert This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@simoami
Copy link
Contributor

simoami commented Mar 8, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The content in the Alert component breaks into several lines when html tags are present in the content. For example: <em>, <i>, <strong>

Screen Shot 2020-03-07 at 11 45 09 PM

Expected Behavior 🤔

The presence of html content should not cause line breaks. Specifically when using html tags aimed at formatting text.

Steps to Reproduce 🕹

import Alert from '@material-ui/lab/Alert'

<Alert severity="warning" icon={false}>You are going to transfer ownership of <strong>Demo Team</strong> to another user, and you will be given Administrator access.</Alert>

Demo: https://codesandbox.io/s/funny-sun-kxmym

Tech Version
Material-UI v4.9.5
Material-UI Lab v4.0.0-alpha.45
React 16.8
Browser Chrome v80
@simoami
Copy link
Contributor Author

simoami commented Mar 8, 2020

The presence of display: flex on the container is causing the html tags in content to break into separate lines.

@simoami simoami changed the title html text formatting in Alert broken [Alert] html content breaks up text in Alert Mar 8, 2020
@mbrookes mbrookes added the component: alert This is the name of the generic UI component, not the React module! label Mar 8, 2020
@oliviertassinari
Copy link
Member

@simoami Any idea on how we could solve this problem?

@simoami
Copy link
Contributor Author

simoami commented Mar 9, 2020

@oliviertassinari There are several ways to fix this but my observation is that the MuiAlert-message class name carries a display: 'flex' prop. Not sure what was the reason, but I can guess it was either meant to vertically center align the message in case the Alert container was given a fixed height (who would do that??) or if the contained action or icon elements were vertically larger than the text content. These are unlikely scenarios in my opinion. and I happen to think that because the icon is aligned to the top, it's better for the text to follow suit. See below visuals for better illustaration:

Action button given a large height to show alignment configuration.

  1. current behavior which breaks html content

Screen Shot 2020-03-08 at 8 13 18 PM

  1. with flex config removed from MuiAlert-message to fix wrapping of text content

Screen Shot 2020-03-08 at 8 15 11 PM

My proposed fix in Alert.js

message: {
  padding: '8px 0',
- display: 'flex',
- flexDirection: 'column',
- justifyContent: 'center',
},

@embeddedt
Copy link
Contributor

Another option is to wrap the inner content in a div or span, as shown in the repro.

@simoami
Copy link
Contributor Author

simoami commented Mar 9, 2020

@embeddedt
Yeah, that's what I did in the demo link above. However, why add nested markup if we don't need to. If we really don't need the flex mode, then we can just drop it and avoid the unnecessary nesting. My suggestion is to address the issue at the root.

@oliviertassinari
Copy link
Member

@simoami Your proposal sounds great, I would propose the following diff:

diff --git a/docs/src/pages/components/alert/DescriptionAlerts.tsx b/docs/src/pages/components/alert/DescriptionAlerts.tsx
index f7a754d8e..da6862d5c 100644
--- a/docs/src/pages/components/alert/DescriptionAlerts.tsx
+++ b/docs/src/pages/components/alert/DescriptionAlerts.tsx
@@ -20,19 +20,19 @@ export default function DescriptionAlerts() {
     <div className={classes.root}>
       <Alert severity="error">
         <AlertTitle>Error</AlertTitle>
-        This is an error alert — check it out!
+        This is an error alert — <strong>check it out!</strong>
       </Alert>
       <Alert severity="warning">
         <AlertTitle>Warning</AlertTitle>
-        This is a warning alert — check it out!
+        This is a warning alert — <strong>check it out!</strong>
       </Alert>
       <Alert severity="info">
         <AlertTitle>Info</AlertTitle>
-        This is an info alert — check it out!
+        This is an info alert — <strong>check it out!</strong>
       </Alert>
       <Alert severity="success">
         <AlertTitle>Success</AlertTitle>
-        This is a success alert — check it out!
+        This is a success alert — <strong>check it out!</strong>
       </Alert>
     </div>
   );
diff --git a/packages/material-ui-lab/src/Alert/Alert.js b/packages/material-ui-lab/src/Alert/Alert.js
index 42111972c..fe62631a3 100644
--- a/packages/material-ui-lab/src/Alert/Alert.js
+++ b/packages/material-ui-lab/src/Alert/Alert.js
@@ -123,9 +123,6 @@ export const styles = theme => {
     /* Styles applied to the message wrapper element. */
     message: {
       padding: '8px 0',
-      display: 'flex',
-      flexDirection: 'column',
-      justifyContent: 'center',
     },
     /* Styles applied to the action wrapper element if `action` is provided. */
     action: {

The update of the demo can servers as a visual regression test. Do you want to send a pull request?
Regarding the motivation for these styles in the first place, well, if nothing visuall changes after removing them, I believe it's safe to ignore :).

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work good first issue Great for first contributions. Enable to learn the contribution process. labels Mar 19, 2020
@kmhigashioka
Copy link
Contributor

Is this up for PR?

@developerKumar
Copy link

Someone working on it? Or I can start?

@oliviertassinari
Copy link
Member

@developerKumar Feel free to go ahead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: alert This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants