fix: font.css not auto-generated from spectrum-css #319
Conversation
| let result = ''; | ||
|
|
||
| for (const srcPath of srcPaths) { | ||
| let data = fs.readFileSync(srcPath, 'utf8'); |
There was a problem hiding this comment.
Just did readFileSync here to keep things simple.
scripts/spectrum-vars.js
Outdated
| // fonts.css (2 sources so a little tricky) | ||
| { | ||
| const srcPath1 = path.join(typographyPath, 'font.css'); | ||
| const srcPath2 = path.join(commonsPath, 'fonts.css'); |
There was a problem hiding this comment.
Now that I'm looking at the output again, I'll swap these so the variables are defined before they're used...
| } | ||
|
|
||
| return result; | ||
| }; |
There was a problem hiding this comment.
put the guts of processCSS here so I could re-use.
package.json
Outdated
| @@ -89,6 +89,7 @@ | |||
| "@open-wc/polyfills-loader": "^0.3.2", | |||
| "@open-wc/testing": "^2.3.3", | |||
| "@open-wc/testing-karma": "^3.1.24", | |||
| "@spectrum-css/commons": "^2.0.0", | |||
There was a problem hiding this comment.
Please add this to the styles package, rather than at the root.
It might make sense to move some of the scripting there over the course as well, but that's something we can get into later.
| --spectrum-font-fallbacks-mono: monospace; | ||
| --spectrum-font-family-ar: myriad-arabic, | ||
|
|
||
| /* Not used |
There was a problem hiding this comment.
Should we upstream an issue to get this removed at the Spectrum CSS level?
packages/styles/fonts.css
Outdated
| governing permissions and limitations under the License. | ||
| */ | ||
|
|
||
| :root { |
There was a problem hiding this comment.
We may need to look at how we're using this... :root won't work in a element, IIRC, which means it'll need to be updated to :root, :host to work at that level.
There was a problem hiding this comment.
@adixon-adobe This may be the only note that must be solved before we move forward, but hopefully you have a minute to get into this soon as I'd love to have this updated.
There was a problem hiding this comment.
Agreed -- I missed this in the conversion. I'm gonna write up a spectrum-css issue and just hard-code this final fix.
There was a problem hiding this comment.
We process this out in other contexts. .spectrum -> :root, :host...we should probably add that so we can keep up-to-date. It would be great if we could get Spectrum CSS to update, but I'd not bet very much money on it...
There was a problem hiding this comment.
oh, and question, is this a bug in spectrum-css, or something we should be converting?
There was a problem hiding this comment.
They don't really trade in :host as it's a Shadow DOM selector, so our conversion...until we convince them to only make a web component distribution of their code 🤔 😜
There was a problem hiding this comment.
thanks -- wasn't clear where things were at there.
scripts/spectrum-vars.js
Outdated
| } | ||
|
|
||
| result = `${license}\n/* stylelint-disable */\n${result}\n/* stylelint-enable */`; | ||
| fs.writeFile(dstPath, result, 'utf8'); |
There was a problem hiding this comment.
I hope we don't have to edit this again, but can we centralize these two lines (same as 65 and 67) just in case we need to alter the structure of the result in the future?
|
Oh, the branch looks a little out of date, once you've rebased to maser I can approve again! |
Added the @spectrum-css/commons package since we need to source our fonts.css from 2 file to get what we need. Also updated the generation script to allow building from multiple source files. Fixes #308
Fix :root selector, move @spectrum-css/commons dependency to styles, and DRY file writing logic
adixon-adobe
force-pushed
the
adixon/308-font-css
branch
from
61eb952 to
547f00d
Compare
|
Rebased and applied the |
Description
Added the @spectrum-css/commons package since we need to source our
fonts.css from 2 file to get what we need. Also updated the generation
script to allow building from multiple source files.
Related Issue
Fixes #308
Motivation and Context
Let's get rid of manual changes needed to update spectrum css.
How Has This Been Tested?
I ran the visual tests locally, and also did some testing on the local doc site (including a bit of manual inspection to make sure the styles I expected to see were there).
I haven't done a deep inspection though, since I'm still not sure how much this affects.
Screenshots (if appropriate):
Types of changes
Checklist: