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

Icon postcard block completed with approval from John on the design #104

Merged
merged 11 commits into from
Apr 30, 2018

Conversation

AlishaOber
Copy link
Contributor

No description provided.

@josephgknox josephgknox self-assigned this Apr 25, 2018
@josephgknox
Copy link

josephgknox commented Apr 25, 2018

Minor tweak to margin call in mixin for consistency; removed two extra spaces in template file. This looks good.

@josephgknox josephgknox requested a review from sherakama April 25, 2018 22:18
@josephgknox
Copy link

@sherakama let me know if you're good with merging, too.

@@ -0,0 +1,10 @@
<div class="decanter-icon-block {{ classes }}">
<img class="decanter-icon-block__img" src="../../../img/circle.png" alt="Please provide alt text">
Copy link
Member

Choose a reason for hiding this comment

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

Please update the src attribute to include a template variable so we can add our own src files when we use this template elsewhere.

Please also do the same with the alt text.

Joe Knox added 2 commits April 27, 2018 14:24
* Quote tweaks (#105)

* Created quote element within elements directory based on design Kerri made for the Engineering project

* tweak Quote template to make it easier to include directly into themes

* don't hardcode path to quote's image

* update neat-omega, grunt, grunt-sass, jquery, and associated dependencies (#106)

* #107 specify node version (#108)

* removed FA from project; updagraded to latest normalize (#101)
@josephgknox josephgknox force-pushed the icon-postcard-block branch from 168cd3a to e1057df Compare April 27, 2018 21:41
@@ -1,6 +1,6 @@
<div class="decanter-quote">
<img class="decanter-quote__img " src="{{ image }}" alt="{{ altText }}">
<div class="decanter-quote__body ">
<img class="decanter-quote__img" src="{{ imageSource }}" alt="{{ alt }}">
Copy link
Member

Choose a reason for hiding this comment

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

@josephgknox

Do not use camelCase for variable names

The standard is to use underscore. See: https://twig.symfony.com/doc/2.x/coding_standards.html

change imageSource to image_source or better yet the shorter img_source or even shorter src

Choose a reason for hiding this comment

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

For sure! Thanks! Fixed.

@josephgknox
Copy link

@sherakama LMK if you think this is ready to merge.

css/decanter.css Outdated
@@ -4,26 +4,34 @@ html {
*, *::after, *::before {
box-sizing: inherit; }

/*! normalize.css v8.0.0 | MIT License | github.com/necolas/normalize.css */
/*! normalize.css v6.0.0 | MIT License | github.com/necolas/normalize.css */
Copy link
Member

Choose a reason for hiding this comment

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

Looks like normalize got rolled back to v6 here. I will update and re-compile.

"image": "../../../img/headshot.jpg",
"altText": "Jane Doe"
"name": "John Doe",
"detail1": "Detail 1",
Copy link
Member

Choose a reason for hiding this comment

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

@josephgknox There are no detail variables in this template. What are they used for?
Also, bio is now missing.

Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

One last change and then I think this is GTG.

@josephgknox
Copy link

There were several merge conflicts; I thought I corrected all of them. Must’ve incorrectly mixed some older things with new things. Yay for reviews. Will fix this up!

@josephgknox
Copy link

@sherakama updated the template issue.

@sherakama
Copy link
Member

@josephgknox, Looks like you re-rolled with normalize 6 again.

Run npm install to get the latest packages then run grunt sass and grunt styleguide. Commit those changes and push it up.

Template changes look good.

@@ -1,7 +1,7 @@
{
"name": "Jane Doe",
"name": "John Doe",
Copy link
Member

Choose a reason for hiding this comment

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

Jane not John here.

@josephgknox
Copy link

💯

Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

GTG

@sherakama sherakama merged commit fca3f51 into master Apr 30, 2018
@sherakama sherakama deleted the icon-postcard-block branch April 30, 2018 17:33
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