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

189 Alert Component #263

Merged
merged 8 commits into from
Nov 7, 2018
Merged

189 Alert Component #263

merged 8 commits into from
Nov 7, 2018

Conversation

josephgknox
Copy link

@josephgknox josephgknox commented Nov 2, 2018

READY FOR REVIEW

Summary

  • Updated Alert component

Needed By (Date)

  • N/A

Urgency

  • N/A

Steps to Test

  1. Pull this branch and compile styleguide
  2. Review visual of alert component and variants
  3. Review all related code

Affected Projects or Products

  • Decanter

Associated Issues and/or People

See Also

@josephgknox josephgknox changed the title initial alert commit WIP 189 Alert Component Nov 2, 2018
//
// Markup: ../templates/components/alert/alert.twig
//
// Style guide: Components.Alert
//
.su-alert {
@include alerts;
}
@include margin(1.5em null null);
Copy link
Member

Choose a reason for hiding this comment

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

I only have one question - is there a reason to add a top margin for this component? We don't have margins for cards or hero block for example. Everything else looks great to me!

Copy link
Member

Choose a reason for hiding this comment

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

Oh good catch!

@josephgknox What gives?

Copy link
Author

Choose a reason for hiding this comment

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

Just keeping you both on your toes. :)

Really, this shouldn't be there, as this type of spacing/margin should be applied more globally (theme level, project level, etc.) rather than on an individual component.

Thanks for catching this. I've pushed up a fix.

@yvonnetangsu yvonnetangsu merged commit 9620631 into master Nov 7, 2018
yvonnetangsu added a commit that referenced this pull request Nov 9, 2018
* master:
  189 Alert Component (#263)
  216 Create site search component (#262)
  266: Update scss file to reference logo.twig and update twig file to … (#267)
  246 homepage (#258)
JBCSU added a commit that referenced this pull request Nov 9, 2018
* master:
  89 browsersync (#272)
  189 Alert Component (#263)
  216 Create site search component (#262)
  266: Update scss file to reference logo.twig and update twig file to … (#267)
  246 homepage (#258)
  reversed order of modular scale type mixin (#260)
  122 hero (#259)

# Conflicts: - all resolved using master
#	core/scss/homepage.md
#	kss/builder/decanter/index.twig
#	kss/builder/decanter/kss-assets/css/kss.css
#	kss/builder/decanter/scss/_home.scss
#	kss/builder/decanter/scss/kss.scss
@sherakama sherakama deleted the 189-alerts branch November 15, 2018 15:14
JBCSU added a commit that referenced this pull request Nov 21, 2018
* master:
  89 browsersync (#272)
  189 Alert Component (#263)

# Conflicts:
#	package-lock.json - resolved using master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants