search

OHIF / Viewers

OHIF zero-footprint DICOM viewer and oncology specific Lesion Tracker, plus shared extension packages
https://docs.ohif.org/
MIT License
3.12k stars 3.29k forks source link

Include Roboto font via WebPack #724

Closed swederik closed 5 years ago

swederik commented 5 years ago

In the Meteor version we eventually switched from Google fonts to locally including the Roboto font. We should do this again. Here was the commit originally: https://github.com/OHIF/Viewers/commit/87d61298b438604140a671cfb03449ae36498f58. This was helpful because otherwise testing the viewer when offline meant that it would load very slowly since it kept trying to reach out to Google to get the fonts.

WebPack can be used to import the font:

https://webpack.js.org/guides/asset-management/#loading-fonts

This will simplify script-tag usage, since this font will no longer need to be pulled from Google Fonts.

dannyrb commented 5 years ago

@swederik, I would actually prefer we don't bundle the font, unless it's for script tag usage. For PWA, having it as a separate include allows us to offload the request to Google's lightning fast CDN and leverage caching that many users will have from having requested that same font for a different website (and Roboto is popular).

For PWA offline scenarios, we can make sure our service worker has a copy of assets we load from Google's CDN. If the asset is fresh enough, or if we're offline, we can use the locally cached copy instead.

This keeps our bundle size lower, improves load times, and makes it easier to not lock users into a specific font.

swederik commented 5 years ago

That's fine with me too, I just don't want the end user for script-tag usage to have to include the fonts in their HTML page, I don't really have a strong opinion on the solution.

dannyrb commented 5 years ago

For Script Tag:

Let's embed. It's one less thing for the user to care about, and we're not as concerned with performance tuning there.

For PWA:

We'll need to modify it's WebPack build.

Caching WebPack application files is probably outside the scope of this issue. There are a few hurdles to navigate carefully, and we don't want to over-complicate this ticket.

dannyrb commented 5 years ago

Additional guidance:

For bundling w/ the commonjs/umd build, import the font in index-umd.js. The entry point file for that bundle.

It should load and bundle. If not, you'll need to add a webpack loader that supports the font's file format.

For PWA, look at my most recent comment on that issues. It's mostly about setting the correct caching rules for the service-worker.