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

change Sass configuaration #205

Merged
merged 32 commits into from
Aug 12, 2022
Merged

change Sass configuaration #205

merged 32 commits into from
Aug 12, 2022

Conversation

dvdherron
Copy link
Contributor

@dvdherron dvdherron commented Apr 19, 2022

Steps to test/reproduce

  • Sass files should be loading Accoutrement from a @forwarded configured version (from tools or config
  • Check the Styleguide to make sure previews and content are loading as they should
  • Check the site diffs to see if any layout issues have been introduced
  • Some of the colors have been split into separate modules. Check /cascading-colors/ and [data-ccs-colors] to make sure everything is loading as it should
    Please explain how to best reproduce the issue and/or test the changes locally (including the pages/URLs/views/states to review).

Show me

Provide screenshots/animated gifs/videos if necessary.

REMEMBER: Attach this PR to the Trello card

@github-actions
Copy link

View diff of compiled files (may take a few minutes): https://github.com/oddbird/oddleventy-built/compare/main..sass-config

@dvdherron
Copy link
Contributor Author

@dvdherron
Copy link
Contributor Author

@mirisuzanne @stacyk This is mostly changing the way we load Accoutrement in our files, similar to what we in Time Designer.

In the Styleguide, I'm wondering how you two feel about the ordering. Is there a way to specify when each item appears if we want the order to be Color Config --> Brand Colors -->> Background Colors . . .etc in a similar order to the current styleguide?

I haven't split animation or scale yet but if that's something we want to do I can start on that here or split those off into separate stories.

@dvdherron dvdherron marked this pull request as ready for review June 15, 2022 13:45
@dvdherron dvdherron requested review from mirisuzanne and stacyk June 15, 2022 13:45
@mirisuzanne
Copy link
Member

In the Styleguide, I'm wondering how you two feel about the ordering. Is there a way to specify when each item appears if we want the order to be Color Config --> Brand Colors -->> Background Colors . . .etc in a similar order to the current styleguide?

I don't think there's any explicit way to set the order of a page. The options would be splitting things into separate pages, or ensuring they have the order we want in the Sass itself. (Sassdoc/herman builds the page in the order things are written in the Sass)

src/scss/config/color/_background.scss Outdated Show resolved Hide resolved
src/scss/config/color/_config.scss Outdated Show resolved Hide resolved
/// @access private
/// @group config-color
$_hues: (
'prime': math.div(color.hue(brand.$brand-blue), 1deg),
Copy link
Member

Choose a reason for hiding this comment

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

Filed an issue to make it so we don't have to do this unit math: oddbird/cascading-color-system#26

src/scss/initial/_colors.scss Outdated Show resolved Hide resolved
src/scss/initial/_colors.scss Outdated Show resolved Hide resolved
src/scss/patterns/_icons.scss Outdated Show resolved Hide resolved
Copy link
Member

@stacyk stacyk left a comment

Choose a reason for hiding this comment

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

@mirisuzanne I made some changes to address your review and make this project more inline with how we have OddBooks structured. I believe we have to use CSS variables for some of the CCS stuff to work, and Herman doesn't have access to the definitions of those colors right? I added and removed and re-added the

@include herman.add(
  'colors',
  'x-colors',
  tools.compile-colors(meta.module-variables('all-other-color-files'))
);

for all the content, background, and ccs default color files (brand uses color values to define the colors instead of custom properties so those are fine). I know the maps would display the color name and value before this change even though it didn't show a preview. If I add that block above for each color file, the colors will show up like a preview, list the color name and the variable value... but all of them are empty. Is there a way load the styleguide.css file like it does when you use an example like the focus-ring styleguide page? https://www.oddbird.net/styleguide/config-focus

src/scss/config/color/_config.scss Outdated Show resolved Hide resolved
src/scss/patterns/_icons.scss Outdated Show resolved Hide resolved
src/scss/initial/_colors.scss Show resolved Hide resolved
@stacyk stacyk requested a review from mirisuzanne August 1, 2022 21:30
* main: (26 commits)
  Remove markdown from u-url links
  fix sample filters
  Upgrade deps
  Click to copy header anchor links (#206)
  Address review
  fix tag
  Fix confusing u-url, and add explicit syndication links
  Update content/blog/2022/shared-elements.md
  Hero image and todays date
  Review for clarity
  review
  Missing files
  Draft page-transitions blog post
  review
  Address review comments
  merge duplication & blocklist filters
  simplify map of syndications
  prefer feed over cache in case of duplicates
  latest node-fetch
  fix macros
  ...
* main:
  Delete snyk.yml
Copy link
Member

@mirisuzanne mirisuzanne left a comment

Choose a reason for hiding this comment

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

looks good! (noted an update we could make to the linter (or the… lintee?), but doesn't seem urgent)

src/scss/config/color/_ccs-defaults.scss Outdated Show resolved Hide resolved
src/scss/initial/_colors.scss Show resolved Hide resolved
* main:
  remove ELEVENTY_ENV env var
  remove duplicate quotes on Metecho and MetaDeploy
  Test if context is readable
  discreet -> discrete
  removed word 'modern' from in front of CSS/Sass
  Update content/blog/2022/zero-units.md
  Link to stylelint option
  Address review comments on zero-units
  review
  Hero image and edits for zero-units
  review
  review
  added david reed and brandon parker quotes to Metecho and Metadeploy
  Replace Google Analytics with Plausible
  Initial draft of zero-units blog post
src/scss/config/color/_index.scss Outdated Show resolved Hide resolved
src/scss/initial/_colors.scss Outdated Show resolved Hide resolved
///
/// @each $name, $color in tools.compile-colors(meta.module-variables('content')) {
/// /*! #{$name}: #{$color}; */
/// }
@include herman.add(
'colors',
'content-colors',
Copy link
Member

Choose a reason for hiding this comment

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

@mirisuzanne doesn't this content-colors here reference the /// @colors content-colors line that we no longer have from the src/scss/config/color/_content.scss file?

Copy link
Member

Choose a reason for hiding this comment

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

uh, yes, I guess so - that line is adding the colors to Herman, even though (for now) we're not displaying them with Herman. So it's not doing anything useful, but also not causing any issues. We could remove that herman.add for now, or just leave it so it works once we add custom property support to Herman? Either way.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I'd say we can leave it there for now since it doesn't hurt anything and maybe if/when we update Herman we will be in good shape.

Copy link
Member

@stacyk stacyk left a comment

Choose a reason for hiding this comment

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

ok @mirisuzanne @jgerigmeyer this has all the colors showing values instead of the blank previews (although the /*! is a bit messy but I think that is so it doesn't error). This will work until we have a better fix in place for Herman.

Screen Shot 2022-08-11 at 11 46 57 AM

The ordering of things on this page doesn't bother me too much but I think it we want a certain order (like having the h1 at the top) we need different pages and I think I prefer them all one one page more than my desire to reorder at this time.

@mirisuzanne
Copy link
Member

@stacyk The /*! comment */ syntax come from Sass, and I just default to using it when we want to see the comment in the output. It seems like maybe we don't need that, since we're not doing compressed output? https://sass-lang.com/documentation/syntax/comments#in-scss

@stacyk
Copy link
Member

stacyk commented Aug 11, 2022

@stacyk The /*! comment */ syntax come from Sass, and I just default to using it when we want to see the comment in the output. It seems like maybe we don't need that, since we're not doing compressed output? https://sass-lang.com/documentation/syntax/comments#in-scss

OK, I updated the comment (removed the !). Is this ready to merge now @mirisuzanne?

Copy link
Member

@mirisuzanne mirisuzanne 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!

@jgerigmeyer jgerigmeyer merged commit 81813b6 into main Aug 12, 2022
@jgerigmeyer jgerigmeyer deleted the sass-config branch August 12, 2022 13:36
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.

5 participants