replace leaflet files from assets.rb in questionRich.html and _mapDependencies.html

[Tlazypanda]

Fixes #7922
Replace leaflet files which are already present in asset pipeline by using javascript_include_tag and stylesheet_link_tag. Also removed files which are already served from application.js and application.css. Added some files to assets.rb for precompilation as well.

Edited _mapDependencies as well since some files there also needed to be replaced and in testing inorder to reduce js execution time.

• 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

Before

After

Thanks!

[codecov]

Codecov Report

Merging #7945 into main will decrease coverage by 0.88%.
The diff coverage is 71.19%.

@@            Coverage Diff             @@
##             main    #7945      +/-   ##
==========================================
- Coverage   82.17%   81.28%   -0.89%     
==========================================
  Files         100      101       +1     
  Lines        5772     5872     +100     
==========================================
+ Hits         4743     4773      +30     
- Misses       1029     1099      +70     

Impacted Files	Coverage Δ
app/controllers/home_controller.rb	98.38% <ø> (ø)
app/controllers/users_controller.rb	81.11% <ø> (-1.19%)	⬇️
app/mailers/admin_mailer.rb	90.90% <0.00%> (ø)
app/models/concerns/statistics.rb	96.72% <ø> (ø)
app/controllers/spam2_controller.rb	61.53% <58.90%> (-38.47%)	⬇️
app/controllers/batch_controller.rb	67.96% <67.96%> (ø)
app/models/user.rb	87.68% <77.77%> (+1.23%)	⬆️
app/controllers/notes_controller.rb	83.40% <83.33%> (-0.01%)	⬇️
app/models/comment.rb	77.51% <83.33%> (+0.62%)	⬆️
app/controllers/wiki_controller.rb	85.34% <92.30%> (+0.34%)	⬆️
... and 32 more

[Tlazypanda]

@jywarren @cesswairimu Should we add the remaining files of /lib mentioned in this file for precompilation by augmenting the assets.rb array? Thanks ✌️

[Tlazypanda]

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

[Tlazypanda]

@VladimirMikulic can you help me with the travis tests failure? Thanks ✌️

[VladimirMikulic]

Hi @Tlazypanda. The error in Travis tells us that the route /assets/images/layers.png doesn't exist.
Leaflet lib is using those layers. You've probably removed the wrong asset. leaflet.js maybe?

[Tlazypanda]

Hmm..weird I removed those assets which would otherwise be available from the asset pipeline @VladimirMikulic leaflet.js is in application.js then shouldn't it be present in the asset pipeline already bundled in application.js 😅

[jywarren]

I believe layer.png often causes issues in the asset pipeline, actually! Maybe this helps? #2537

But i recall this kind of thing with Leaflet image assets, esp. those referred to in CSS, because CSS doesn't respect asset pipeline routing (i think that's right!). So it could be worth a broader search for leaflet layers.png asset pipeline -- maybe on the Leaflet repo or StackOverflow?

Good luck, this looks great and thanks to both of you!

[Tlazypanda]

@jywarren Found the issue! So basically whenever we are trying to move this into the asset pipeline it is searching in the route assets/images for the css images but in the link form it is searching in the node_modules folder where the css file is ...so a fix would be to move the layers.png file to assets/images but I am not sure if I should proceed with it ...what are your thoughts on this? Thanks ✌️

[Tlazypanda]

Hey @cesswairimu @jywarren any update on what should be my course of action 😅 Thanks ✌️

[jywarren]

OK this could be linked to #5095 (comment) -- is that right?

For the broader question, originally we included assets in the editor because we thought it optimal to only include them here, instead of on any page (the editor has an especially large number of assets, not needed elsewhere on the site). But I totally trust your judgement and analysis of whether re-organizing which assets are loaded where and how they're minified, so as long as nothing breaks 😅 please go for it! Thank you!!!

[Tlazypanda]

Hey @jywarren @cesswairimu I am also fairly confused as to what can be done here 😅 The reason being if I am supposed to reference the images without adding them to assets/images route then I will have to add the path of the specific images dir to the config.assets.path but the issue with this is that I will have to reference the image css with image_url(image.png) but since this is an external library I can't change its code. Maybe we can remove the css file from the asset pipeline to prevent adding these static images for precompilation?

https://guides.rubyonrails.org/asset_pipeline.html#coding-links-to-assets
https://makandracards.com/makandra/29567-managing-vendor-libraries-with-the-rails-asset-pipeline

[Tlazypanda]

@cesswairimu @jywarren can you kindly review? Thanks ✌️

[cesswairimu]

@Tlazypanda quick question...are the contents of the files in /assets identical to the ones in /lib?

[Tlazypanda]

Hey @cesswairimu I am not sure I completely follow 😅 on precompilation all are put together in assets directory but since here we are referencing image from css we need to add relative path so since this is an external lib and we can't make changes to the code (since it is yarn package) so I removed the file that required the css leaflet.css (as you can see in _mapDependencies now it is referenced from /lib suggesting that is not part of asset pipeline)

The scripts removed are already present in the application.js

[cesswairimu]

aha! gotcha thanks 👍

[sagarpreet-chadha]

i am approving it but maybe you might want to do this change in follow up PR or here only. No worries, thanks 😄

[sagarpreet-chadha]

I think we should keep this and remove from application level, because as you are also working on performance improvement, having a light entry page should take less time, right?
When these libraries are required they should be loaded only then (lazy loading), what do you think?
😄 🎉

[jywarren]

Hi @Tlazypanda any comment on these last questions from @sagarpreet-chadha ? My apologies for replying late to these. Thank you!!!

[Tlazypanda]

Hey @jywarren @sagarpreet-chadha I totally missed this 😅 my apologies ...ummm can we do this in a follow up pr because I think the leaflet.js and css files are actually required on many routes since the map is loaded on dashboard/user profile/all editor pages/map routes etc so might have to investigate it more to make sure that removing it won't break anything. What do you think? Thanks ✌️

[jywarren]

That's totally fine. Always game to split up PRs! Does that make this PR otherwise good to merge? Thank you!

[jywarren]

Retriggering once more!

[jywarren]

Great work, @Tlazypanda !!!