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.

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

Refactor mobile javascript to get rid of console errors. #5470

Merged
merged 1 commit into from Jan 17, 2015

Conversation

@jaywink
Copy link
Contributor

commented Dec 13, 2014

Fixes also mobile bookmarklet services.

Basically:

  • Went through the whole mobile app checking for console errors
  • Extracted mobile file uploader to own file, since it's only needed in status message creation
  • Made sure "main" JS does not need to be loaded in status message creation by including I18n JS directly, instead of having to rely on the rest of the JS to run
  • Removed unnecessary duplicate loading of mobile.js on a few pages

I would have liked to move the inline JS to JS files and maybe reorganize a bit better so includes could be used- but I think that should be another PR. This fixes actual bugs, ie bookmarklet service icons have not been working for a long time.

- if current_page?(:new_status_message)
= javascript_include_tag "helpers/i18n"
= load_javascript_locales
= javascript_include_tag "mobile/mobile_file_uploader"

This comment has been minimized.

Copy link
@jhass

jhass Dec 13, 2014

Member

Given this is the mobile site, I think we should think about this a bit. My intuition would be to always load all JS that's required on any mobile page, and to only use one javascript_include_tag and sprockets requires for the other tags. The reason is that I think that requests are more expensive than a few kb extra that will be cached on the next request anyway, especially on mobile.

This comment has been minimized.

Copy link
@jaywink

jaywink Jan 17, 2015

Author Contributor

Yeah I think it's better now? Hopefully cukes will be happy too..

@jaywink jaywink referenced this pull request Dec 21, 2014

@jaywink jaywink force-pushed the jaywink:mobile-js-fixes branch 3 times, most recently from b831860 to 30b4264 Jan 17, 2015

@@ -61,7 +62,8 @@ class Application < Rails::Application
mailchimp.js
main.js
jsxc.js
mobile.js
mobile/mobile.js
mobile/mobile_file_uploader.js

This comment has been minimized.

Copy link
@jhass

jhass Jan 17, 2015

Member

Only list files here that you need to have available for javascript_include_tag and the like, everything else will get pulled in via the sprockets requires.

@jaywink

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2015

"One more (Travis cukes) time..." ...

It is possible #5489 fixed the cukes which I broke by rearranging stuff 💃

Refactor mobile javascript to get rid of console errors.
Fixes also mobile bookmarklet services.

@jaywink jaywink force-pushed the jaywink:mobile-js-fixes branch from 30b4264 to 9282a4e Jan 17, 2015

@jaywink

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2015

Right, tuned according to comments and tests now pass.

@jhass jhass added this to the next-major milestone Jan 17, 2015

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 17, 2015

Thank you!

@jhass jhass merged commit 9282a4e into diaspora:develop Jan 17, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
jhass added a commit that referenced this pull request Jan 17, 2015
Merge pull request #5470 from jaywink/mobile-js-fixes
Refactor mobile javascript to get rid of console errors.

@jaywink jaywink deleted the jaywink:mobile-js-fixes branch Jan 17, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.