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

Minify bootstrap slider script and add defer tag #7939

Merged
merged 2 commits into from Feb 23, 2021

Conversation

Tlazypanda
Copy link
Collaborator

Fixes #7918
Bootstrap slider script minified and defer tag added to allow execution after html parsing (not used async because this script requires jquery).

  • 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 Tlazypanda requested a review from a team as a code owner May 21, 2020 06:19
@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

❗ No coverage uploaded for pull request base (main@b2742cc). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7939   +/-   ##
=======================================
  Coverage        ?   81.81%           
=======================================
  Files           ?      100           
  Lines           ?     5955           
  Branches        ?        0           
=======================================
  Hits            ?     4872           
  Misses          ?     1083           
  Partials        ?        0           

@Tlazypanda
Copy link
Collaborator Author

Tlazypanda commented May 26, 2020

@jywarren @cesswairimu @emilyashley @SidharthBansal @VladimirMikulic can you kindly review ? Thanks ✌️

@Tlazypanda
Copy link
Collaborator Author

@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>
Copy link
Member

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!

Copy link
Member

@jywarren jywarren left a 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!

@Tlazypanda
Copy link
Collaborator Author

Tlazypanda commented Jun 4, 2020

Hey @jywarren @cesswairimu ! So while going through the function and testing it out here are some observations-
These observations are from publiclab.org and not local

  • This is invoked by adding location from dashboard but on clicking share location from profile page nothing happens.
  • On adding location modal, the slider works by zooming in but when we save it and check it out in the profile page it is saved without any zoom.
  • Also after adding your location it still shows add location on dashboard
  • Also after adding location for first time, it doesn't seem to update for a second time
  • Some weird alert comes sometimes couldn't catch it but it was something like owning not found ...

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 ✌️

@cesswairimu
Copy link
Collaborator

Great findings here @Tlazypanda 👍 I will try and reproduce and let you know what I find. Thanks

@Tlazypanda
Copy link
Collaborator Author

@cesswairimu @jywarren Adding more to the weird alert (last point from above comment) - this time took a screenshot from publiclab.org and not local
err

@jywarren
Copy link
Member

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 owmloading -- i've seen that before! I believe it was an assets issue related to an upstream Leaflet library last time. This time, take a look and see if we could have caused it another way? #5095 (comment)

@Tlazypanda
Copy link
Collaborator Author

Hey @jywarren @cesswairimu ! So while going through the function and testing it out here are some observations-
These observations are from publiclab.org and not local

  • This is invoked by adding location from dashboard but on clicking share location from profile page nothing happens.
  • On adding location modal, the slider works by zooming in but when we save it and check it out in the profile page it is saved without any zoom.
  • Also after adding your location it still shows add location on dashboard
  • Also after adding location for first time, it doesn't seem to update for a second time
  • Some weird alert comes sometimes couldn't catch it but it was something like owning not found ...

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

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 👻

@Tlazypanda
Copy link
Collaborator Author

Hey @cesswairimu @jywarren are the above behaviours reproducible 😅 Thanks ✌️

@stale stale bot added the stale label Oct 7, 2020
@stale stale bot removed the stale label Oct 7, 2020
@publiclab publiclab deleted a comment from stale bot Oct 13, 2020
@jywarren
Copy link
Member

Sorry! Ok;

  • use from dashboard - clicking Add your location gets me the below, so is it on actually saving that it doesn't do anything for you? I confirm the Save button doesn't do anything (at least visible to the user!)

image

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 break-me-up?

  • On adding location modal, the slider works by zooming in but when we save it and check it out in the profile page it is saved without any zoom.
  • Also after adding your location it still shows add location on dashboard
  • This is invoked by adding location from dashboard but on clicking share location from profile page nothing happens.
  • Also after adding location for first time, it doesn't seem to update for a second time

@jywarren
Copy link
Member

We also ought to add a system test for adding profile tags - like, one of these modified for the dashboard!

test 'adding a location to the wiki' do
visit '/wiki/wiki-page-path/'
# Toggle "Add location" modal
find('a.btn-location').click()
# Enter a location
find('#map_content #coord_button').click()
find('#map_content #lat').set("22")
find('#map_content #lng').set("76")
# Save the location
find('#blurred-location-modal .btn-primary').click()
# Wait for the location to be added
wait_for_ajax
# there should also be a page reload now, because we want to reload to see the sidebar mini-map
find('a#tags-open').click()
# Make sure proper latitude and longitude tags are added
assert_selector('.tags-list .badge a[href="/tag/lat:22"]', text: "lat:22")
assert_selector('.tags-list .badge a[href="/tag/lon:76"]', text: "lon:76")
end

We could add one of these assertions afterwards that checks that the profile tags are properly created:

test 'adding a tag to a user profile' do
visit "/profile/jeff"
# run the javascript function
page.evaluate_script("addTag('specialgroup', '/profile/tags/create/2')")
find('#tags-section').click
# check that the tag showed up on the page - check last tag in list
within('.tags-list') do
assert_equal('specialgroup', all('.tag-name').last.text.rstrip.lstrip)
end

The ideal workflow is:

  1. See "add a location" on dashboard, click it
  2. See modal and check content of one or more of the inputs
  3. enter lat/lon and zoom, or use the slider
  4. press Save and see an alert (maybe?) that says "profile location was saved. Learn more about location privacy" - linking to https://publiclab.org/location-privacy
  5. confirm dashboard button now says "View nearby content
  6. confirm tags were added and appear on profile page

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:

image

I'l move the parts about the dashboard button into its own issue!

Thanks @Tlazypanda !!

@jywarren
Copy link
Member

Great, moved dashboard stuff to #8566! Thanks!

@gitpod-io
Copy link

gitpod-io bot commented Feb 23, 2021

@codeclimate
Copy link

codeclimate bot commented Feb 23, 2021

Code Climate has analyzed commit bc1ee98 and detected 0 issues on this pull request.

View more on Code Climate.

@jywarren jywarren merged commit 03e9b27 into publiclab:main Feb 23, 2021
lagunasmel pushed a commit to lagunasmel/plots2 that referenced this pull request Mar 2, 2021
Co-authored-by: Jeffrey Warren <jeff@unterbahn.com>
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
Co-authored-by: Jeffrey Warren <jeff@unterbahn.com>
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
Co-authored-by: Jeffrey Warren <jeff@unterbahn.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace bootstrap-slider.js with minified js and add defer tag
4 participants