-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Minify bootstrap slider script and add defer tag #7939
Conversation
Codecov Report
@@ Coverage Diff @@
## main #7939 +/- ##
=======================================
Coverage ? 81.81%
=======================================
Files ? 100
Lines ? 5955
Branches ? 0
=======================================
Hits ? 4872
Misses ? 1083
Partials ? 0 |
@jywarren @cesswairimu @emilyashley @SidharthBansal @VladimirMikulic can you kindly review ? Thanks ✌️ |
@VladimirMikulic Thanks a lot for the review 🎉 |
<script src="https://cdnjs.cloudflare.com/ajax/libs/bootstrap-slider/10.2.0/bootstrap-slider.js"></script> | ||
</style> | ||
|
||
<script defer src="https://cdnjs.cloudflare.com/ajax/libs/bootstrap-slider/10.2.0/bootstrap-slider.min.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, i'm just trying to think of where this is used so that it's clear it'll still work!
See how you can use the "blame" view to find out: https://github.com/publiclab/plots2/blame/master/app/views/layouts/application.html.erb#L95
Given it links here: #4633
Could you test that function to be sure, and just note that in this PR? That'll give us extra confidence! But this does look good to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just modifying this to a "request changes", thanks!
Hey @jywarren @cesswairimu ! So while going through the function and testing it out here are some observations-
So while this pr won't pose a problem(since it is a performance improvement) but first maybe we should work these things out to get a better understanding of the adding location workflow.Can someone else try to reproduce and help me out? Thanks ✌️ |
Great findings here @Tlazypanda 👍 I will try and reproduce and let you know what I find. Thanks |
@cesswairimu @jywarren Adding more to the weird alert (last point from above comment) - this time took a screenshot from publiclab.org and not local |
Thank you, i agree you've found some issues with the workflow and would love help in correcting them! Perhaps we can run this by @ebarry once you establish what you think the workflow /ought/ to be, just for a +1? and we can for sure do that all in another issue! As for |
Hey @jywarren @cesswairimu @ebarry can you try reproducing these? Once I am confirmed that this is actually what's happening we can start establishing the workflow in a new issue 😅 And fix issues if any with the slider and all As for the mysterious owmloading thing I have left a comment at #5095 😅 Hopefully will be able to set the ghost free 👻 |
Hey @cesswairimu @jywarren are the above behaviours reproducible 😅 Thanks ✌️ |
Sorry! Ok;
Just checked, and I can confirm your observations on all of this. I think we should try to break this list out into some bugfix issues, using
|
We also ought to add a system test for adding profile tags - like, one of these modified for the dashboard! plots2/test/system/post_test.rb Lines 188 to 211 in 86776c2
We could add one of these assertions afterwards that checks that the profile tags are properly created: plots2/test/system/tag_test.rb Lines 40 to 51 in 86776c2
The ideal workflow is:
This is really too much for this PR, so let's create a new issue around it! The more basic usage would be when adding locations using the "Add a location" button in the tags input on pages like https://stable.publiclab.org/notes/anngneal/12-08-2017/7-ways-to-measure-monitor-and-evaluate-water-quality -- we can test this out there to finish this PR: I'l move the parts about the dashboard button into its own issue! Thanks @Tlazypanda !! |
Great, moved dashboard stuff to #8566! Thanks! |
Code Climate has analyzed commit bc1ee98 and detected 0 issues on this pull request. View more on Code Climate. |
Co-authored-by: Jeffrey Warren <jeff@unterbahn.com>
Co-authored-by: Jeffrey Warren <jeff@unterbahn.com>
Co-authored-by: Jeffrey Warren <jeff@unterbahn.com>
Fixes #7918
Bootstrap slider script minified and defer tag added to allow execution after html parsing (not used async because this script requires jquery).
rake test
@publiclab/reviewers
for help, in a comment belowThanks!