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.

Sign up for GitHub

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

Jump to bottom

TASK: Extract fonts to load seperatelty #821

Merged
merged 2 commits into from
Aug 31, 2017
Merged

TASK: Extract fonts to load seperatelty #821

merged 2 commits into from
Aug 31, 2017

Conversation

mstruebing
Copy link
Contributor

@mstruebing mstruebing commented Aug 13, 2017 •
edited
Loading

closes #816

@dimaip

@mstruebing mstruebing requested a review from dimaip August 13, 2017 11:00
@dimaip
Copy link
Contributor

dimaip commented Aug 14, 2017 •
edited
Loading

Hey! What about that remark that Jonas made? https://neos-project.slack.com/messages/C0U0KEGDQ/convo/C0U0KEGDQ-1502348659.634050/

As said, I'm not up to date on the topic. E.g. maybe our browser support would allow us to only ship woff2? That would reduce the download size.

@mstruebing
Copy link
Contributor Author

I've decided to include woff because of: http://caniuse.com/#search=woff vs http://caniuse.com/#search=woff2

IE11 isn't supporting woff2, but I think many of NeosCMS users has to be IE11 compatible.

I'm not up to date which would be better either. But I can make a research on that topic.

Maybe @jonas-siegl can say something more detailed about that?

@mstruebing
Copy link
Contributor Author

mstruebing commented Aug 15, 2017 •
edited
Loading

This is the only useful resource I could find right now: https://www.bramstein.com/writing/web-font-anti-patterns-inlining.html

But maybe Bram isn't telling us everything.

Also if the user already have the font the browser didn't have to redownload the font again which wouldn't be possible with inlining.

@dimaip
Copy link
Contributor

dimaip commented Aug 15, 2017

Yeah, I had suspected something like this, just wasn't sure enough to insist. Also we can cache fonts very aggressively, but that's not something we want to do with CSS.
I would just merge your PR, but wonder what @jonas-siegl and other more experienced frontend devs think. Maybe @aertmann has an opinion?

grebaldi
grebaldi reviewed Aug 15, 2017
View reviewed changes
packages/build-essentials/src/webpack.config.js Outdated
@@ -33,8 +33,8 @@ const webpackConfig = {
loader: 'json-loader'
},
{
test: /\.(woff|woff2|eot|svg)$/,
loader: 'url-loader?limit=100000'
test: /\.(woff|woff2)$/,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this loader can be dropped, since it is removed from the package.json.

Any reasons, why it's still here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current implementation of font-awesome.

https://github.com/neos/neos-ui/tree/57b0655ef67c52168c3298869b1a2906a711eefe/packages/react-ui-components/src/Icon/fontAwesome

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case you shouldn't remove the loader from the dependencies, right? https://github.com/neos/neos-ui/pull/821/files/b0242e333c0d12bf7fbeb031e3c6fe10a2831143#diff-6e183e897d30da3e86a99a9c16857780L52

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was working like this before I implemented the typeface-work-sans npm module and it is working again, the loader was added because of errors during the build with this module, why should I add a dependency I don't need?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because you are still using it. The loader "url?limit=100000" is a shortcut for "url-loader?limit=100000". Webpack automatically appends "-loader" to it when the first module lookup failed. Maybe it's still working because another package has the module "url-loader" as a dependency.
Anyway I would recommend to not use the short syntax because it can lead to strange issues if there is another module called "url". Therefore this logic has been removed in Webpack 2 by default. (see webpack/webpack#2986)

@jonas-siegl
Copy link
Contributor

Hey,
I'm sorry for my late reply and the trouble. For now I would also recommend to remove the inlining completely, because it's not useful in combination with the current webfont integration approach.
There are more advanced loading techniques to fight FOIT and FOUT where it make sense to use it.
Here is a great article where you can read more about webfont loading strategies: https://www.zachleat.com/web/comprehensive-webfonts/

dimaip
dimaip suggested changes Aug 17, 2017
View reviewed changes
Copy link
Contributor

@dimaip dimaip left a comment

Choose a reason for hiding this comment

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

Ok, so let's finish this PR the way it is. Left some comments.

Configuration/Settings.yaml Outdated
@@ -34,6 +34,15 @@ Neos:
'Neos.Neos.UI:HostOnlyStyles':
resource: '${"resource://" + Neos.Ui.StaticResources.compiledResourcePackage() + "/Public/Styles/HostOnlyStyles.css"}'
position: 'start 900'
'WorkSans:200':
Copy link
Contributor

Choose a reason for hiding this comment

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

Ehh you can't load the font like this, it ain't no webpack... ;)
You need to do smth like this CSS:

@font-face {
  font-family: "WorkSans";
  src: url("/...pathetofonts/work-sans-latin-200.woff") format("woff");
  font-weight: normal;
  font-style: normal;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if we switched from NotoSans to WorkSans (why?) you need to also change the family here: https://github.com/neos/neos-ui/blob/master/packages/neos-ui/src/styleHostOnly.css#L8

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, do we really need all three weights? As far as I can see in the UI we only use normal and bold weights.

Copy link
Contributor Author

@mstruebing mstruebing Aug 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know why we switched. As I first picked up font stuff it were already work-sans.
These is were I picked up:
#761
#690

I will fix the weights and body-font tomorrow morning.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dimaip We switched because of this, I think: https://github.com/neos/brand/blob/2f2fe306f0ce1c5ee2ca7345c51464134cfad247/index.js#L23

Is it possible, that the brand assets are used for both, the website and the UI?

Copy link
Contributor

Choose a reason for hiding this comment

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

@grebaldi what do you mean by brand assets?

I don't mind WorkSans, we just have to use it here: https://github.com/neos/neos-ui/blob/master/packages/neos-ui/src/styleHostOnly.css#L8
Preferably by using brand constants of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

@grebaldi @dimaip the referenced "brand-assets" is a package where the colors and fonts of Neos are defined, and yes they where used in the website back when I started it, this difference is ofc now kind of problematic.

Was the change in font families meant to be just for the website or the whole brand of Neos? In the latter case ofc it makes sense to also change it in the UI doesnt it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Inkdpixels hey nice to see you here again!
I'm sure it was just a mistake, we should use the definition from the brands package ofc.

@dimaip
Copy link
Contributor

dimaip commented Aug 31, 2017

Can we then remove the worksans npm package?

@mstruebing
Copy link
Contributor Author

I did: https://github.com/neos/neos-ui/pull/821/files#diff-2fce5f75e3e31d94e254e4466f727a84L92

@dimaip
Copy link
Contributor

dimaip commented Aug 31, 2017

Oh, sorry, missed it!

@mstruebing
Copy link
Contributor Author

So, everything alright then? :)

@dimaip
Copy link
Contributor

dimaip commented Aug 31, 2017

As you see, I approved your PR, but didn't push the green button myself, as I'm scared to take the final responsibility 😆
Go ahead and push the button!

@mstruebing mstruebing merged commit b6b85c4 into neos:master Aug 31, 2017
@mstruebing
Copy link
Contributor Author

boom-2028563_960_720

@mstruebing mstruebing deleted the task/816/fontHandling branch May 7, 2018 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Inkdpixels Inkdpixels Inkdpixels left review comments

@jonas-siegl jonas-siegl jonas-siegl left review comments

@grebaldi grebaldi grebaldi left review comments

@dimaip dimaip dimaip approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mstruebing @dimaip @jonas-siegl @Inkdpixels @grebaldi