-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
A few additions for migration to webpack #781
Conversation
* Fix issues on paths in build and dev files * Skip yarn to reduce tooling requirements * Minify CSS assets in production * Reduce imports on fonts, hardcode font pieces more. There are problems with the font family names. Roboto mixin brings in everything as `Roboto-Slab-Bold`, which won't match local font name of 'Roboto Slab' * Fix the development instance more: * Add file watching on reST files * Execute build docs command after webpack build Things I'm still not sure about: - [ ] The actual JS output, I haven't vetted this yet - [ ] If woff2 and woff are enough for fonts. The NPM packages are missing other formats
I should also report that the dev server mostly works. It rebuilds docs on changes to reST files, it uses the correct assets (afaict), and it refreshes on updates. |
outputPath: 'fonts/', | ||
publicPath: '/_static/fonts', | ||
publicPath: '../fonts/', |
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.
I'm still a little confused by this. It feels like publicPath
is applied in a backwards way. I feel it should describe how paths are altered for the MiniCssExtractPlugin
instances for css/theme.css
, not backwards, where we alter the publicPath
for font files, assuming they are used in the css/theme.css
file with the nested path.
I don't know. I think this is technically correct, it just logically feels backwards. 🤷♂️
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.
That's okay, I'm more than happy for this change
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.
Aye, this seems to work. I think webpack just has me confused 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.
These changes all look good to me. I'm going to leave my approval here.
Re. woff(2) font files, those will work for all browsers except IE 6-8. Either we add .eot/.ttf files for them or they will fall back to some other font: Georgia or Arial for headings and Arial for body text. |
That seems fine I suppose. From the RTD perspective, we have settled on only support IE 11. Browsers < IE 11 represent .78% of our annual traffic. They at least won't have a broken experience. I'd be fine setting similar guidelines for the theme, especially as we've discussed removing things like modernizer. MS doesn't support IE < 11 anymore, so I'm less inclined to try to support these browsers. I'll say for the purposes of testing that we're okay with woff/woff2. I'm going to poke the JS output a little (i didn't see any errors on my testing so far), and if it looks good, I'll merge back to #771. |
@agjohnson good to hear about IE. I think users and maintainers of the theme would benefit from some written statement in the theme documentation about supported browsers. |
That's our implicit goal, but I agree we should explicitly state our support. It might be worth periodic audits of browsers to see where we stand, I know I haven't tested a number of browsers in a while. I've opened #786 to discuss this next. We can follow up there and write some docs. I'm going to push up the 1.9 pin and will merge this into #771. After I merge the translation branches, some docs can be updated in #771 |
with the font family names. Roboto mixin brings in everything as
Roboto-Slab-Bold
, which won't match local font name of 'Roboto Slab'Things I'm still not sure about:
Based on #771