fix(inline-notification): add 8px right padding if close button is hidden #4873

jendowns · 2019-12-13T16:01:18Z

Right now if you remove the close button in the InlineNotification (as shown in screenshots for #4864), the action button will be flush against the right side of the notification. For the low-contrast variation, this flush right margin is problematic because it overlaps the 1px low-contrast border.

~~This PR proposes adding a 1px right margin to the action button so that it won't be "flush" on the right side of the notification & also won't overlap the low-contrast border:~~ Updated to 8px now

Changelog

Changed

add 8px right padding to the InlineNotification if hideCloseButton={true}

netlify · 2019-12-13T16:10:44Z

Deploy preview for the-carbon-components ready!

Built with commit 66dbc79

https://deploy-preview-4873--the-carbon-components.netlify.com

netlify · 2019-12-13T16:14:21Z

Deploy preview for carbon-components-react ready!

Built with commit 66dbc79

https://deploy-preview-4873--carbon-components-react.netlify.com

netlify · 2019-12-13T16:15:44Z

Deploy preview for carbon-elements ready!

Built with commit 66dbc79

https://deploy-preview-4873--carbon-elements.netlify.com

netlify · 2019-12-13T16:22:05Z

Deploy preview for the-carbon-components ready!

Built with commit 0618c4a

https://deploy-preview-4873--the-carbon-components.netlify.com

netlify · 2019-12-13T16:31:39Z

Deploy preview for carbon-components-react ready!

Built with commit 0618c4a

https://deploy-preview-4873--carbon-components-react.netlify.com

netlify · 2019-12-13T16:31:59Z

Deploy preview for carbon-elements ready!

Built with commit 0618c4a

https://deploy-preview-4873--carbon-elements.netlify.com

laurenmrice

If there is no close icon can we adjust the action button to be 8px away from the right hand side.

…4_action-btn

jendowns · 2019-12-13T21:51:09Z

Thank you @laurenmrice! I just committed an update to the right spacing so that should be reflected in the previews soonl. 👍

@joshblack I ended up changing how I added spacing... since the 8px spacing would only be present if hidenCloseButton={true}, I moved that logic to the component js. Could you take another look at what I did when you get a chance & let me know if you agree? Thank you!

asudoh

Thank you very much @jendowns for fixing!

asudoh · 2019-12-13T23:38:26Z

packages/react/src/components/Notification/Notification.js

+        role={role}
+        kind={kind}
+        className={classes}
+        style={{ paddingRight: hideCloseButton ? spacing03 : '0' }}>


Any reason you didn't do this via Sass code (and update className upon hideCloseButton)...?

Thanks @asudoh -- with the current structure of this component, the action button is a sibling to the close button & therefore it's difficult to target & change the action button when the close button is no longer present.

Adding a class is definitely a good alternative approach. I just went with the solution that (seemed to) add/remove the least. Would you prefer to add a class and keep the padding update in Sass only?

Thank you for sharing your thoughts @jendowns! I'm thinking about the ability of style override by applications, which tends to lean toward Sass approach. Is it changing className upon hideCloseButton enough to tell the Sass code that the close button is no longer present? If my words here is not clear enough please don't hesitate to speak up - Thanks!

That's a good point @asudoh -- I just committed a change that goes the classname + Sass route. Let me know what you think -- thank you!

asudoh

LGTM 👍 - Thanks @jendowns for the update!

laurenmrice

Action link placement looks good. Can we take off the Example Link that comes after the normal text? I don't know if that happened in this pr, but I would rather only show the action link on the right.

jendowns · 2019-12-16T16:04:45Z

Thanks @laurenmrice! If you don't mind, I was wondering if we could keep those example links there? 😅

I added them in a PR a week or so ago because a change went through that prevented clicking inside the notification body text. Without the example link there previously, the change went through undetected & had an effect on both gatsby-theme-carbon (mentioned here: #4804) and the carbon-website (mentioned here: carbon-design-system/carbon-website#655).

OH and I should add: technically since that notification body, or specifically the subtitle, accepts a node type, it can have a link passed into it by a user. Which is what we were seeing on both the Gatsby theme and the Carbon official website.

So keeping that link there could help catch any future issues with links in the body. 😅 What do you think?

laurenmrice

Okay we can keep it there thanks for explaining, will approve. I am wondering if it could eventually be something that you could turn off and on from the story? I am just wondering if people will be confused why there is a link and also an action link.

jendowns · 2019-12-16T16:47:26Z

@laurenmrice Yeah that's a good question... I'm honestly not sure if storybook knobs can allow you to customize HTML elements (specifically the <a> element for this example link) which may make it hard to customize that InlineNotification body/subtitle nodes 🤔

BUT another option: I could add a storybook variation? Something you select on the sidebar? Right now there's Toast and Inline... there could be a new one like Inline without example link or the inverse, Inline with example link -- too much?

laurenmrice · 2019-12-16T16:53:12Z

I think we can leave it for now and I can revisit that eventually :) thanks jen!

…dden (carbon-design-system#4873)

fix(inline-notification): add 1px right margin to action btn

66dbc79

jendowns requested a review from a team as a code owner December 13, 2019 16:01

ghost requested review from dakahn and joshblack December 13, 2019 16:01

Merge branch 'master' into 4864_action-btn

dc6f95d

joshblack approved these changes Dec 13, 2019

View reviewed changes

joshblack requested a review from a team December 13, 2019 16:09

ghost requested review from laurenmrice and removed request for a team December 13, 2019 16:09

laurenmrice requested changes Dec 13, 2019

View reviewed changes

jendowns added 3 commits December 13, 2019 15:48

fix(inline-notification): use 8px right padding when close button hidden

19bf1e8

Merge branch '4864_action-btn' of github.com:jendowns/carbon into 486…

309a293

…4_action-btn

Merge branch 'master' into 4864_action-btn

1b321ac

jendowns changed the title ~~fix(inline-notification): add 1px right margin to action btn~~ fix(inline-notification): add 8px right padding if close button is hidden Dec 13, 2019

asudoh reviewed Dec 13, 2019

View reviewed changes

jendowns added 2 commits December 13, 2019 18:45

fix(inline-notification): apply margin w class when close btn hidden

1b7fdc2

Merge branch 'master' into 4864_action-btn

5026d5e

asudoh approved these changes Dec 14, 2019

View reviewed changes

jendowns requested a review from laurenmrice December 16, 2019 14:56

laurenmrice requested changes Dec 16, 2019

View reviewed changes

joshblack approved these changes Dec 16, 2019

View reviewed changes

laurenmrice approved these changes Dec 16, 2019

View reviewed changes

dakahn approved these changes Dec 16, 2019

View reviewed changes

laurenmrice added the status: ready to merge 🎉 label Dec 16, 2019

jendowns and others added 2 commits December 16, 2019 16:22

Merge branch 'master' into 4864_action-btn

39618ae

Merge branch 'master' into 4864_action-btn

0618c4a

asudoh merged commit 36b9762 into carbon-design-system:master Dec 16, 2019

joshblack pushed a commit to joshblack/carbon that referenced this pull request Jan 6, 2020

fix(inline-notification): add 8px right padding if close button is hi…

16f50a6

…dden (carbon-design-system#4873)

joshblack mentioned this pull request Jan 6, 2020

chore(project): v10.9.1 release #4957

Closed

joshblack mentioned this pull request Jan 31, 2020

chore(project): update carbon to v10.10.0-rc.0 #5236

Closed

joshblack mentioned this pull request Feb 25, 2020

chore(release): v10.10.0 #5432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(inline-notification): add 8px right padding if close button is hidden #4873

fix(inline-notification): add 8px right padding if close button is hidden #4873

jendowns commented Dec 13, 2019 •

edited

Loading

netlify bot commented Dec 13, 2019

netlify bot commented Dec 13, 2019

netlify bot commented Dec 13, 2019

netlify bot commented Dec 13, 2019 •

edited

Loading

netlify bot commented Dec 13, 2019 •

edited

Loading

netlify bot commented Dec 13, 2019 •

edited

Loading

laurenmrice left a comment

jendowns commented Dec 13, 2019

asudoh left a comment

asudoh Dec 13, 2019 •

edited

Loading

jendowns Dec 14, 2019 •

edited

Loading

asudoh Dec 14, 2019

jendowns Dec 14, 2019

asudoh left a comment

laurenmrice left a comment

jendowns commented Dec 16, 2019 •

edited

Loading

laurenmrice left a comment

jendowns commented Dec 16, 2019 •

edited

Loading

laurenmrice commented Dec 16, 2019

fix(inline-notification): add 8px right padding if close button is hidden #4873

fix(inline-notification): add 8px right padding if close button is hidden #4873

Conversation

jendowns commented Dec 13, 2019 • edited Loading

Changelog

netlify bot commented Dec 13, 2019

netlify bot commented Dec 13, 2019

netlify bot commented Dec 13, 2019

netlify bot commented Dec 13, 2019 • edited Loading

netlify bot commented Dec 13, 2019 • edited Loading

netlify bot commented Dec 13, 2019 • edited Loading

laurenmrice left a comment

Choose a reason for hiding this comment

jendowns commented Dec 13, 2019

asudoh left a comment

Choose a reason for hiding this comment

asudoh Dec 13, 2019 • edited Loading

Choose a reason for hiding this comment

jendowns Dec 14, 2019 • edited Loading

Choose a reason for hiding this comment

asudoh Dec 14, 2019

Choose a reason for hiding this comment

jendowns Dec 14, 2019

Choose a reason for hiding this comment

asudoh left a comment

Choose a reason for hiding this comment

laurenmrice left a comment

Choose a reason for hiding this comment

jendowns commented Dec 16, 2019 • edited Loading

laurenmrice left a comment

Choose a reason for hiding this comment

jendowns commented Dec 16, 2019 • edited Loading

laurenmrice commented Dec 16, 2019

jendowns commented Dec 13, 2019 •

edited

Loading

netlify bot commented Dec 13, 2019 •

edited

Loading

netlify bot commented Dec 13, 2019 •

edited

Loading

netlify bot commented Dec 13, 2019 •

edited

Loading

asudoh Dec 13, 2019 •

edited

Loading

jendowns Dec 14, 2019 •

edited

Loading

jendowns commented Dec 16, 2019 •

edited

Loading

jendowns commented Dec 16, 2019 •

edited

Loading