search

oddbird / accoutrement

Combined Sass Accoutrement tools, with option for individual module imports
MIT License
37 stars 6 forks source link

Docs cleanup #97

Closed dvdherron closed 2 years ago

dvdherron commented 2 years ago

Description

A lot of changes were made for the upcoming release. This PR tries to make sure that

Steps to test/reproduce

I've been scanning related closed PRs to get an overview of what has changed.

These are the ones where the bulk of the changes were made:

  1. Modules Core
  2. Modules Init
  3. Type
  4. Color
  5. Scale
  6. Layout
  7. Animate

Please explain how to best reproduce the issue and/or test the changes locally (including the pages/URLs/views/states to review).

dvdherron commented 2 years ago

I want Accoutrement on Sass Modules

dvdherron commented 2 years ago

@mirisuzanne @stacyk This is ready to start reviewing.

Some questions and known issues:

If you you notice anything else missing or or weird just let me know

mirisuzanne commented 2 years ago

@dvdherron

Do we want to remove all Eyeglass references? I think all mentions of it in the modules are removed, there are just references in the README and the Changelog.

I don't know. We still support eyeglass in the npm package. It just seemed weird to highlight it, as one of many possible package managers/bundlers? Maybe there's some way to just note in one place that tools like eyeglass or webpack will impact the import path? @jgerigmeyer may have better ideas how/where to note that?

Should the "Pre-Registered Functions" section show in the public docs? I left a question above about why that might not be working.

I… can't figure this out. Nested folders shouldn't make any difference here, but yes - when I move the files, they suddenly appear in the docs. @jgerigmeyer any idea why that might be? We can move all the files, I'd just like to understand what's going on.

In the map-compile-with example (under tokens/compile) I'm not sure why the compiled result is returning null

There are two possible solutions:

Probably the first option is simplest?

If you you notice anything else missing or or weird just let me know

I'll do a full review here in a bit…

jgerigmeyer commented 2 years ago

Do we want to remove all Eyeglass references? I think all mentions of it in the modules are removed, there are just references in the README and the Changelog.

I don't know. We still support eyeglass in the npm package. It just seemed weird to highlight it, as one of many possible package managers/bundlers? Maybe there's some way to just note in one place that tools like eyeglass or webpack will impact the import path? @jgerigmeyer may have better ideas how/where to note that?

I don't have a strong feeling about this. It seems like what's there isn't hurting anything (although the Eyeglass example in the README seems identical to the non-Eyeglass example), but I'm also fine removing the specifics since Eyeglass/Webpack users would likely know how to import npm packages. 🤷

Should the "Pre-Registered Functions" section show in the public docs? I left a question above about why that might not be working.

I… can't figure this out. Nested folders shouldn't make any difference here, but yes - when I move the files, they suddenly appear in the docs. @jgerigmeyer any idea why that might be? We can move all the files, I'd just like to understand what's going on.

Fixed in https://github.com/oddbird/accoutrement/pull/97/commits/2f0e2d16bb24f52fcab9334a9264df8888103472 (see https://medium.com/@jakubsynowiec/you-should-always-quote-your-globs-in-npm-scripts-621887a2a784).

dvdherron commented 2 years ago

@dvdherron

Do we want to remove all Eyeglass references? I think all mentions of it in the modules are removed, there are just references in the README and the Changelog.

I don't know. We still support eyeglass in the npm package. It just seemed weird to highlight it, as one of many possible package managers/bundlers? Maybe there's some way to just note in one place that tools like eyeglass or webpack will impact the import path? @jgerigmeyer may have better ideas how/where to note that?

Yeah, like Jonny said I guess having at least one mention of it doesn't hurt. Since we removed on all of the modules I just wanted to double check that we hadn't merely overlooked the remaining references.

Should the "Pre-Registered Functions" section show in the public docs? I left a question above about why that might not be working.

I… can't figure this out. Nested folders shouldn't make any difference here, but yes - when I move the files, they suddenly appear in the docs. @jgerigmeyer any idea why that might be? We can move all the files, I'd just like to understand what's going on.

Jonny's fix got these to show. I'll double check to make sure everything there is looking okay.

In the map-compile-with example (under tokens/compile) I'm not sure why the compiled result is returning null

* The example runs a map through the `color()` function.

* The color function looks for each of those color names in the `tools.$colors` map

* The colors are not defined in `tools.$colors`, only in the example map

There are two possible solutions:

* change `$brand-colors` to be `tools.$colors`, so that the color function can find them

* pass in `$brand-colors` as a `$source` argument, the same way we do in `compile-colors`

Probably the first option is simplest?

Makes sense. Cleaned this up and it's compiling correctly now.

If you you notice anything else missing or or weird just let me know

I'll do a full review here in a bit…

👍🏽

dvdherron commented 2 years ago

@mirisuzanne I think these changes cover all of your review comments.