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 duplicate from assets.rb already present in application.js #7958

Merged
merged 1 commit into from
Aug 18, 2020

Conversation

Tlazypanda
Copy link
Collaborator

Fixes #7889

Duplicate js files already present in application.js removed from assets.rb.

  • 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

Thanks!

@Tlazypanda
Copy link
Collaborator Author

@jywarren @cesswairimu @emilyashley @SidharthBansal @VladimirMikulic can you kindly review? Also since ics is already present as a package in /lib as well as in app/asssets/javascripts from where should I remove it? Since it is a library package it makes more sense to remove it from the /javascripts directory. Thanks ✌️

@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #7958 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7958   +/-   ##
=======================================
  Coverage   81.39%   81.39%           
=======================================
  Files         101      101           
  Lines        5859     5859           
=======================================
  Hits         4769     4769           
  Misses       1090     1090           

Copy link
Member

@Uzay-G Uzay-G left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good and the system tests are passing 👍

@Tlazypanda
Copy link
Collaborator Author

@Uzay-G Thanks a ton for the review! :octocat:

@Tlazypanda
Copy link
Collaborator Author

@jywarren @cesswairimu @emilyashley @SidharthBansal @VladimirMikulic can you kindly review? Also since ics is already present as a package in /lib as well as in app/asssets/javascripts from where should I remove it? Since it is a library package it makes more sense to remove it from the /javascripts directory. Thanks

Hey @jywarren @cesswairimu Need your input on this 😅

@Tlazypanda
Copy link
Collaborator Author

Hey @cesswairimu Can you kindly review? This shouldn't break anything but I am a bit worried 😅

@cesswairimu
Copy link
Collaborator

I understand cool I am pulling this and will see if I can see anything locally..also @Tlazypanda if you come across any issue that can be an fto..kindly ping @nsachin08..I can also help converting it to a first-timer issue..haven't been able to get one yet.

@jywarren
Copy link
Member

jywarren commented Jul 7, 2020

trying to restart these tests!!

@jywarren
Copy link
Member

jywarren commented Jul 7, 2020

OK, we look good here! I'm not sure about ics -- i guess i trust your judgement!!! 🎉

If you think this is good, let's move ahead! Do you want to try testing it out on stable?

@jywarren
Copy link
Member

jywarren commented Jul 7, 2020

er, unstable?

@Tlazypanda
Copy link
Collaborator Author

Hey @jywarren have pushed on unstable now can you help me test just to be sure I don't miss anything? Thanks ✌️

@Tlazypanda
Copy link
Collaborator Author

Tlazypanda commented Jul 7, 2020

@jywarren @cesswairimu I have tested some routes out on unstable seems alright ...can you also crosscheck once because i might have missed something ? 😅 Thanks ✌️

Copy link
Contributor

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow awesome, if tested rigourously we can merge this! Thanks 🎈 😄

@gitpod-io
Copy link

gitpod-io bot commented Aug 18, 2020

@jywarren
Copy link
Member

Rebased to check on GitPod!

@jywarren jywarren merged commit 4cced45 into publiclab:main Aug 18, 2020
piyushswain pushed a commit to piyushswain/plots2 that referenced this pull request Oct 22, 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.

Check for js files duplication in assets.rb and application.js
6 participants