Skip to content

too heavy on mobile #475

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

Open
jaroslaw-weber opened this issue Oct 25, 2017 · 10 comments
Open

too heavy on mobile #475

jaroslaw-weber opened this issue Oct 25, 2017 · 10 comments
Labels
A-JavaScript Area: Javascript A-Mobile Area: Mobile A-Performance Area: Zoom Zoom

Comments

@jaroslaw-weber
Copy link

mdbook has lot of features but its getting heavy on mobile. i was able to make a static website with most of the functionality and go down to 70kb (cookbook is 250, rust book is over 300) so i think there is a room for improvement.

what can we do:

  • remove jquery
  • create custom icon pack
  • ... ?
@Michael-F-Bryan Michael-F-Bryan added A-JavaScript Area: Javascript A-Mobile Area: Mobile A-Performance Area: Zoom Zoom labels Nov 18, 2017
@sorin-davidoi
Copy link
Contributor

Would a PR that removes jQuery (uses native DOM APIs instead) be accepted?

@Michael-F-Bryan
Copy link
Contributor

Michael-F-Bryan commented Jan 12, 2018

It'd be really nice to get rid of the JQuery dependency. Besides the fact that it makes the generated website smaller it'll also make packaging mdbook a lot easier on platforms where installers aren't allowed to access the internet.

In particular @tjis (#271 (comment)) and @infinity0 (#495) have had issues trying to package Rust on Debian because The Book uses mdbook.

@steveklabnik
Copy link
Member

steveklabnik commented Jan 12, 2018 via email

@sorin-davidoi
Copy link
Contributor

^ Is in response to my question (why don't bundle jQuery locally) which I shouldn't have deleted.

@infinity0
Copy link

To clarify on @steveklabnik's answer, the preferred solution for distros is if there were options to allow linking to locally-installed versions of JS libraries (including jquery) rather than remote-loading from a CDN all the time. Bundling copies of dependencies together with the repo would not be helpful - but also wouldn't hinder packaging; we'd just ignore those copies and link to locally-installed ones where possible.

Also jQuery is not a problem for Debian, it is already packaged, it has no dependencies and uses a simple standard buildsystem. It's other NPM packages with masses of pointless 10-liner dependencies and special-snowflake-buildsystems that are the problem.

@sorin-davidoi
Copy link
Contributor

What about polyfills? Are those fair to bundle? I cannot image Debian packaging every polyfill out there.

@infinity0
Copy link

That depends on the packager and the specific situation. I for one would reuse an existing polyfill that's already packaged by Debian, e.g. es6-promise, instead of packaging a new Promise polyfill. Then I'd add a Debian-specific patch to make that package use es6-promise. Usually it's just a few lines in package.json. (I've done this a few times already.)

@steveklabnik
Copy link
Member

@sorin-davidoi oops, hehe, I replied via email so I didn't see it.

@Michael-F-Bryan
Copy link
Contributor

Looking through the comments made from @steveklabnik and @infinity0 it seems like my earlier thought of "remove all the JS dependencies" was a bit too wide-sweeping. If it's easy enough for Debian to do a ln -s to override the vendored JQuery before mdbook gets compiled, then there's no longer any reason to remove it.

@sorin-davidoi, what are your thoughts?

@sorin-davidoi
Copy link
Contributor

I am not familiar with the packaging process - my view on this is that removing jQuery is good for the performance of the page (less JS to load, parse and keep in memory).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-JavaScript Area: Javascript A-Mobile Area: Mobile A-Performance Area: Zoom Zoom
Projects
None yet
Development

No branches or pull requests

5 participants