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

Separate javascript into page-specific files and remove unused code #381

Merged
merged 2 commits into from Aug 18, 2020

Conversation

m52go
Copy link
Contributor

@m52go m52go commented Jul 30, 2020

I wanted to add an anchor link to the front page, but FAQ page JavaScript was getting in the way.

As it turned out, JavaScript code for the home and FAQ pages was being loaded for every page. This also made load times unnecessarily slower for other pages.

This PR puts JavaScript for the home page in a separate home.js file that is only loaded when the home page is loaded. Likewise for the FAQ page.

Additionally, it removes lingering code from when Google Analytics was used.

Review/test:
https://deploy-preview-381--bisq-website.netlify.app/

Review Guidance

In this case, there are just 2 commits, so it's worth looking at them separately.

  • 288eef8 just removes the sendAnalytic() function and both lines that call it
    • there isn't much to test here, aside from maybe inspecting the rest of the code base for any lingering mentions of sendAnalytic()...since these proposed changes remove that function, that method should not be used anywhere else.
  • 0605baa separates JavaScript in /js/scripts.js into 2 separate files called /js/home.js and /js/faq.js.
    • one way to test this is to make sure that all dynamic elements on the home page and faq page still work (e.g., accordion expanders on faq page, hover effect on getting-started section of front page, etc)
    • another good check could involve diffing the _site/ folder before and after these proposed changes are put in place. Since nothing visual should have changed, the generated website should be almost identical. To do this, just build the site without proposed changes, build the site with proposed changes, and then compare the result:
      • run git checkout master, rm -rf _site, and then bundle exec jekyll build.
      • move the resulting _site folder somewhere else.
      • then run git checkout move-js, rm -rf _site, and then bundle exec jekyll build.
      • compare the resulting _site folder with the other one with a diffing tool of your choice (e.g. Meld).

You should see js/scripts.js missing on every page, and js/home.js and js/faq.js added to all home pages and all faq pages...but everything else should be the same.

@Bayernatoor
Copy link
Member

288eef8:

  1. Reviewed the code base - found no other mention of the sendAnalytic() function.

0605baa:

  1. Dynamic elements checked and working on local page and netlify, expanders and hover effect. (did notice that i couldn't switch to light mode, only dark mode was available).

  2. Compared _site dir from master against proposed branch (correct term? ). script.js has been removed and replaced and added to the js directory home.js and faq.js

Looks good to me ... let me know if i went about it wrong at any point :)

Not sure whether the light mode/dark mode not working is an issue.

JavaScript for home and faq pages now have their own files, and
site-wide JavaScript is now in site.js.
@m52go
Copy link
Contributor Author

m52go commented Aug 18, 2020

(did notice that i couldn't switch to light mode, only dark mode was available).

This was crucial. Thank you for finding this. Yes I completely removed the code for the toggling dark mode when deleting scripts.js...the code for the toggle was not commented and kind of hidden at the bottom, so I didn't see it.

I've added it back now and rebased the branch so it's included in the last commit (ec7f8ea).

@m52go m52go merged commit ede2115 into bisq-network:master Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants