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

remove cdn links from new.html.erb which are available from asset pip… #7943

Merged
merged 1 commit into from
Jun 2, 2020

Conversation

Tlazypanda
Copy link
Collaborator

…eline

Fixes #7917
Since the asset pipeline already had some assets available so removed those cdn links.
Tested by

  • setting the asset.compress mode to True and assets.debug mode to false
  • removing the cdn links and checking if it affected the upload csv option
  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

Screenshots attached -

Before
new html

After
new html_After

Thanks!

@codecov
Copy link

codecov bot commented May 22, 2020

Codecov Report

Merging #7943 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7943   +/-   ##
=======================================
  Coverage   82.04%   82.04%           
=======================================
  Files          97       97           
  Lines        5642     5642           
=======================================
  Hits         4629     4629           
  Misses       1013     1013           
Impacted Files Coverage Δ
app/controllers/stats_controller.rb 73.33% <100.00%> (ø)

@Tlazypanda Tlazypanda closed this May 22, 2020
@Tlazypanda Tlazypanda reopened this May 22, 2020
@Tlazypanda
Copy link
Collaborator Author

@jywarren @cesswairimu just to confirm the chart.js and popper.js is the same as the ones we already have right ?

Also for popper.js, it is present in package.json but not called anywhere except this file. So it might be better to serve it from a cdn since it will be faster this way 😅

Thanks ✌️

@Tlazypanda
Copy link
Collaborator Author

Hey @jywarren @cesswairimu @emilyashley Looking forward to your input on this 😄

@jywarren
Copy link
Member

Hi!! Sorry for slow reply... if we're not around also please see if other Outreachy/GSoC folks are able to help review?

As to Popper, i believe it's a Bootstrap dependency?

Looking now though! Thanks so much!!!

@jywarren
Copy link
Member

This looks good to me! Perhaps @IshaGupta18 may want to chime in, otherwise we are good to merge this!

@Tlazypanda
Copy link
Collaborator Author

@jywarren Thanks so much for the review ❤️

@cesswairimu cesswairimu merged commit 4cc367d into publiclab:master Jun 2, 2020
@cesswairimu
Copy link
Collaborator

Thanks @Tlazypanda 🎉

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.

Remove cdn links for assets already available from pipeline in app/views/csvfiles/new.html.erb
3 participants