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

Jump to bottom

D8CORE-2490: All new alerts #725

Merged
merged 18 commits into from
Sep 1, 2020
Merged

D8CORE-2490: All new alerts #725

merged 18 commits into from
Sep 1, 2020

Conversation

sherakama
Copy link
Member

@sherakama sherakama commented Aug 27, 2020 •
edited
Loading

READY FOR REVIEW

Summary

  • Updates NPM and all the packages
  • Updates to font-awesome fonts and icons
  • Updates to the Alerts component.
  • Close functionality for alert.

Needed By (Date)

  • Sept 3rd

Urgency

  • Hight

Steps to Test

test.twig

<div class="su-stuff">
  <h2>Things and stuff.</h2>
  <p>More things and stuff.</p>

  {% include "@decanter/components/alert/alert.twig" %}

  {% set alert_label = "Success" %}
  {% set modifier_class = "su-alert--success su-alert--text-light" %}
  {% include "@decanter/components/alert/alert.twig" %}

  {% set alert_label = "Info" %}
  {% set modifier_class = "su-alert--info su-alert--text-light" %}
  {% include "@decanter/components/alert/alert.twig" %}

  {% set alert_label = "Error" %}
  {% set modifier_class = "su-alert--error su-alert--text-light" %}
  {% include "@decanter/components/alert/alert.twig" %}

  {% set alert_label = "Warning" %}
  {% set modifier_class = "su-alert--warning" %}
  {% include "@decanter/components/alert/alert.twig" %}

  <p>&nbsp;</p>
  <p>Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>
</div>
  1. Set up your developer environment and use the above test template
  2. Compile all the assets (npm run publish)
  3. Review the code
  4. Review the responsive breakpoints and the aesthetics compared to https://www.figma.com/file/Kmd4utmJFPRMVeCFEEBQhLtx/Decanter-Design-System?node-id=9316%3A22
  5. Review preview build

Affected Projects or Products

  • D8CORE
  • Everything that used alerts before

Associated Issues and/or People

  • D8CORE-2490
  • Adapt

See Also

@sherakama sherakama temporarily deployed to Tugboat August 27, 2020 16:03 Destroyed
@sherakama sherakama temporarily deployed to Tugboat August 27, 2020 21:29 Destroyed
@yvonnetangsu
Copy link
Member

@sherakama Is this ready for initial review?

@sherakama
Copy link
Member Author

Not final review but if you want to take an initial review please have at it.

@sherakama sherakama requested a review from yvonnetangsu August 27, 2020 22:54
@yvonnetangsu
Copy link
Member

@sherakama I think it's looking great. I like your use of utility classes (like the text-light) since it will upgrade to v7 better, and the use of flex order. Will review more thoroughly when it's ready.

@sherakama sherakama temporarily deployed to Tugboat August 28, 2020 04:52 Destroyed
@sherakama sherakama temporarily deployed to Tugboat August 28, 2020 07:40 Destroyed
@sherakama sherakama temporarily deployed to Tugboat September 1, 2020 05:17 Destroyed
@sherakama
Copy link
Member Author

@yvonnetangsu This is ready for your critique. Thanks.

yvonnetangsu
yvonnetangsu requested changes Sep 1, 2020
View reviewed changes
Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

More review in the morning - looking great!

core/src/scss/components/alert/_alert.scss Outdated Show resolved Hide resolved
yvonnetangsu
yvonnetangsu reviewed Sep 1, 2020
View reviewed changes
Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Just those comments :)

core/src/scss/core/fonts/_fontawesome-fonts.scss Show resolved Hide resolved
core/src/scss/components/alert/_alert.scss
// .su-alert--warning - Warning Message
// .su-alert--error su-alert--text-light - Error Message
// .su-alert--info su-alert--text-light - Informative Message
// .su-alert--text-light - Lighter color text for darker backgrounds
Copy link
Member

Choose a reason for hiding this comment

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

Are we including the alert variant with the big icon in this PR, or just the small icon version?

Screen Shot 2020-09-01 at 10 43 55 AM

Copy link
Member

Choose a reason for hiding this comment

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

FYI looks like there are different padding values in the container depending on whether the alert has a big Alert Header and an Alert Footer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, just the one variant for this release. I hope to come back to the other variants later.

@sherakama sherakama temporarily deployed to Tugboat September 1, 2020 18:05 Destroyed
@sherakama sherakama temporarily deployed to Tugboat September 1, 2020 18:27 Destroyed
@sherakama
Copy link
Member Author

@yvonnetangsu assets and spacing has been adjusted.

@yvonnetangsu
Copy link
Member

yvonnetangsu commented Sep 1, 2020 •
edited
Loading

@sherakama Thanks for the change - this is super close. A couple small things:

  • Might need a max-width (something like width minus the width of the dismiss button or other approach) on the alert__body element
    Screen Shot 2020-09-01 at 12 35 52 PM
  • Looks like we could use a larger padding when there are no alert header and alert footer - maybe a decorator class? (screenshot showing current state - looks a little slim when there are no header and footer)
    Screen Shot 2020-09-01 at 12 36 30 PM

@sherakama sherakama temporarily deployed to Tugboat September 1, 2020 20:59 Destroyed
@sherakama
Copy link
Member Author

  • Looks like we could use a larger padding when there are no alert header and alert footer - maybe a decorator class? (screenshot showing current state - looks a little slim when there are no header and footer)

Yeah, I think one or two of the other variants will help in this case. The big icons or larger text options might be a better fit. I don't mind the slim notice overall.

@sherakama
Copy link
Member Author

Might need a max-width (something like width minus the width of the dismiss button or other approach) on the alert__body element

I just made sure to add some margin between the two on the XS screen size. That is the only size that gets that layout.

@yvonnetangsu
Copy link
Member

  • Looks like we could use a larger padding when there are no alert header and alert footer - maybe a decorator class? (screenshot showing current state - looks a little slim when there are no header and footer)

Yeah, I think one or two of the other variants will help in this case. The big icons or larger text options might be a better fit. I don't mind the slim notice overall.

Yeah, I don't mind it being slim either.

@yvonnetangsu yvonnetangsu self-requested a review September 1, 2020 21:50
yvonnetangsu
yvonnetangsu approved these changes Sep 1, 2020
View reviewed changes
Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Looks great! Good to go 👍

@sherakama
Copy link
Member Author

Winna winna.

@sherakama sherakama merged commit cc4e48a into master Sep 1, 2020
@sherakama sherakama deleted the D8CORE-2490 branch September 1, 2020 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yvonnetangsu yvonnetangsu yvonnetangsu approved these changes

Assignees

@yvonnetangsu yvonnetangsu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sherakama @yvonnetangsu