-
Notifications
You must be signed in to change notification settings - Fork 9
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
Bourbon update #115
Bourbon update #115
Conversation
Gruntfile.js
Outdated
|
||
grunt.registerTask('styleguide', ['run:styleguide', 'verbosity:symlinkquiet']); | ||
grunt.registerTask('default', ['postcss:dist']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicating the default task. Remove this line.
You can, instead, move the 'postcss:dist'
tasks in to the styleguide task array in line 120. You will also probably want to ensure that it is ran after calling grunt sass
but you cannot simply just add this to the end of that command. As you want to make this a dependency I would suggest registering another a new task called compile
or something that runs both grunt sass and grunt post-css in the same command.
eg: grunt.registerTask('compile', ['sass:dist', 'postcss:dist']);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! And, I really like the compile
task idea. I've corrected this and have added that task.
@@ -991,7 +991,7 @@ table { | |||
border-right: 0; } | |||
|
|||
html { | |||
font-family: "Source Sans Pro", "Helvetica Neue", "Helvetica", "Roboto", "Arial", sans-serif; | |||
font-family: "Source Sans Pro", "Helvetica Neue", "Helvetica", "Arial", sans-serif; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like Roboto went away here. Was this intensional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fonts after "Source San Pro"
here are generated by the $font-stack-helvetica
font stack variable. These are fallbacks. Bourbon has updated these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming.
.decanter-serif h1, .decanter-serif | ||
h2, .decanter-serif | ||
h3, .decanter-serif | ||
h4, .decanter-serif | ||
h5, .decanter-serif | ||
h6 { | ||
font-family: "Crimson Text", "Georgia", "Cambria", "Times New Roman", "Times", serif; } | ||
font-family: "Crimson Text", "Georgia", "Times", "Times New Roman", serif; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cambria is missing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fonts after "Crimson Text"
here are generated by the $font-stack-georgia
font stack variable. These are fallbacks. Bourbon has updated these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming.
@@ -1424,7 +1424,7 @@ a.decanter-button--unstyled { | |||
.decanter-icon-block .decanter-icon-block__img { | |||
float: left; | |||
margin-right: 1rem; | |||
margin-right: 1.776889em; } | |||
margin-right: 1.5625em; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this margin change intensional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. modular-scale(n)
is being used on this component (it's the only component utilizing this so far) and it has been updated. We can change it use the margin mixin instead (for now), but there is no real visual difference in the style guide and pattern library as it adjusted relatively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as this change is acceptable to you 👍
@@ -17,21 +17,24 @@ | |||
"url": "https://github.com/SU-SWS/decanter/issues" | |||
}, | |||
"dependencies": { | |||
"bourbon": "^4.2.7", | |||
"bourbon": "^5.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are pretty major changes. Please change the version of decanter to 3.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why we would want to wait until Neat is updated, too? Or, would that be cause for updating to 4.0.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat is a pretty major update as well (By the way, i'm happy to make myself available to answer questions about updating neat. Some of the concepts on how neat handles padding, margins, and containers changed pretty significantly between neat 1.x and the latest stable release (v2.1.0).)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @kgcreative! Sounds like another major release with Neat would be a good idea then. So, I'll update the version now and we can again release a new version with the eventual Neat updates.
Thanks for the offer to help; I'll surely want to pick your brain on how to best make the updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we upgrade Neat, or should we refactor to use CSS Grid? What's the longevity of Neat now that grid is widely supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat 2.x (And the upcoming 3.x) can be used as forward-compatible fallbacks for both flexbox and grid. Had the SU Homepage Launch happened 6 months later, I would have used grid for components. CSS grid and flexbox have so many advantages over floated layouts, which will make most float-based frameworks mostly obsolete. Browser support for both are really good. Neat's breakpoint management is really great, but a lot of grid layouts can handle being responsive by using smart fractional units combinations and zero breakpoints, so even that is a massive improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! We do plan on upgrading Neat as the next major undertaking (version 4.0.0
).
Shea and I have also begun to talk about (and want to meet with you, JB, about) adding CSS Grid and/or Flex Box-based classes that can be used in Decanter in addition to the Neat grid. That is, our template files for pages and other items that can and should be more rigid can leverage the current Decanter grid classes (Neat), but patterns and components can leverage more flexible ones (CSS Grid/Flex Box).
In the short-term, though, to get the project in a place where it is utilizing the latest Bourbon and Neat, I thought that this work should be prioritized first, especially before we start building out too many components and patterns that would need to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments left.
I have reviewed these changes and this PR looks GTG. Please have a review as well. The next steps after approval will be:
|
css/decanter-grid.css
Outdated
@@ -265,6 +266,4 @@ body { | |||
margin-left: 85.2980433528%; } | |||
|
|||
.decanter-offset-eleven-twelfths { | |||
margin-left: 93.8278476881%; } | |||
|
|||
/*# sourceMappingURL=decanter-grid.css.map */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josephgknox
It looks like the source map gets removed with the auto-prefixer. Is this going to cause an error for debugging?
@@ -37,10 +37,21 @@ module.exports = function(grunt) { | |||
} | |||
} | |||
}, | |||
postcss: { | |||
options: { | |||
map: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josephgknox: I have now set this to true in order to preserve the sass maps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sherakama awesome! I had originally set it to false as the sass task was writing the maps. Based on order of operations (and now that these things happened as part of a single command) this should've been set to true. Thanks!
Along with this PR I would like to see an |
@sherakama took a first stab at stubbing out an |
@@ -0,0 +1,43 @@ | |||
# Upgrade |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Thanks @josephgknox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet! Thanks for adding the steps to actually receive the update. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in approving this. Somehow I missed the PR and the request for review.
This is ready to go. Please do the following in order:
I'll put together a meeting to do some backlog grooming and goal setting for next week. Thanks everyone. |
READY FOR REVIEW
Summary
font-face
and Font stack variables)Needed By (Date)
Urgency
Steps to Test
grunt postcss
); also runs withgrunt watch
if running when change is madeAffected Projects or Products
Associated Issues and/or People
See Also