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.

Sign up for GitHub

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

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

Merged
merged 9 commits into from
Dec 16, 2019
Merged

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

Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
66dbc79
fix(inline-notification): add 1px right margin to action btn
jendowns Dec 13, 2019
dc6f95d
Merge branch 'master' into 4864_action-btn
jendowns Dec 13, 2019
19bf1e8
fix(inline-notification): use 8px right padding when close button hidden
jendowns Dec 13, 2019
309a293
Merge branch '4864_action-btn' of github.com:jendowns/carbon into 486…
jendowns Dec 13, 2019
1b321ac
Merge branch 'master' into 4864_action-btn
jendowns Dec 13, 2019
1b7fdc2
fix(inline-notification): apply margin w class when close btn hidden
jendowns Dec 14, 2019
5026d5e
Merge branch 'master' into 4864_action-btn
jendowns Dec 14, 2019
39618ae
Merge branch 'master' into 4864_action-btn
jendowns Dec 16, 2019
0618c4a
Merge branch 'master' into 4864_action-btn
asudoh Dec 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/react/src/components/Notification/Notification.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
CheckmarkFilled20,
WarningFilled20,
} from '@carbon/icons-react';
import { spacing03 } from '@carbon/layout';

import Button from '../Button';

Expand Down Expand Up @@ -504,7 +505,12 @@ export class InlineNotification extends Component {
);

return (
<div {...other} role={role} kind={kind} className={classes}>
<div
{...other}
role={role}
kind={kind}
className={classes}
style={{ paddingRight: hideCloseButton ? spacing03 : '0' }}>
Copy link
Contributor

@asudoh asudoh Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jendowns jendowns Dec 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

<div className={`${prefix}--inline-notification__details`}>
<NotificationIcon
notificationType={notificationType}
Expand Down