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

Added search location feature to archive.html #1286

Merged
merged 3 commits into from
Dec 8, 2022
Merged

Conversation

hs05june
Copy link
Contributor

@hs05june hs05june commented Dec 4, 2022

Fixes #1272 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • 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 grunt test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updates
  • @mention the original creator of the issue in a comment below for help or for a review

Hey @jywarren. I have added search feature to archive.html.

@welcome
Copy link

welcome bot commented Dec 4, 2022

Thanks for opening this pull request! Dangerbot will test out your code and reply in a bit with some pointers and requests.
There may be some errors, but don't worry! We're here to help! 👍🎉😄


One thing that can help to get started is to make sure you've included a link back to the original issue you're solving, in the format fixes #0000 (for example). And to make sure the PR title describes what you're trying to do! (often it can be the same as the issue title) Thanks! 🙌


Then, you can say hello in our chatroom & share a link to this PR to get a review! 👋 ✅

@hs05june
Copy link
Contributor Author

hs05june commented Dec 4, 2022

Hey @jywarren . I have added search loaction feature in archive.html. It works fine in the beginning.
2022-12-04
But as soon as sidebar is opened it doesn't work. I failed to figure that out. Kindly suggest some changes.

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.

Looks great! Let me test it out, hang on a sec...



<i title="Open Sidebar" id="mapToggle" title="Open Sidebar" class="fa fa-bars fa-3x " style="position: absolute; right: 0; top:30px; margin: 1rem; z-index: 900; color: white; cursor: pointer;" aria-hidden="true"></i>

<div class="offcanvas offcanvas-end" data-bs-backdrop="false" data-bs-keyboard="false" tabindex="1" id="offcanvasRight" aria-labelledby="offcanvasRightLabel">
Copy link
Member

Choose a reason for hiding this comment

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

Wow, it is very strange. I have to think maybe some of the click event handlers at the bottom of the file are interfering with this.

But I also noticed that if I remove this whole 'offcanvas' div in the developer console, it then works. So... it's somehow involved?

Copy link
Member

Choose a reason for hiding this comment

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

I can even set the offcanvas CSS visibility: visible; to visibility: hidden; and then the geocoding search box works. Mysterious!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's very strange. @jywarren Since I am new to the community. Can you suggest me any other issue I can work with.

Copy link
Member

Choose a reason for hiding this comment

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

Sure - @segun-codes and @7malikk are just starting their Outreachy fellowship and perhaps one of them may have an idea for a good issue? I'd also like to know what they think of this difficulty we're having. I would hope the fix is really simple, but we just need to identify the mechanism for the error. Thanks for your patience!

Copy link
Collaborator

@7malikk 7malikk Dec 6, 2022

Choose a reason for hiding this comment

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

Hey there @hs05june checkout this issue #1174, I believe it has not been claimed and is a good first issue for you. Thank you for your patience.
In the meantime, @jywarren, I will be looking into this bug and let you both know how it goes, my suspicions are the same as yours, the click events handlers could've posed some restrictions. It's quite strange.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's very strange. @jywarren Since I am new to the community. Can you suggest me any other issue I can work with.

Hi @hs05june, you may find this issue #855 interesting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jywarren from the study I conducted, a visibility: visible style and a show class are appended on the offcanvas element when the offcanvas element is visible but said style is not removed when the modalToggle event listener is triggered. This is due to the setup of the said event listener which is an if statement that checks for the class show and removes it.

The solution to this problem would be:

  • Take out the modalToggle event listener
-188  modalToggle.addEventListener('click', (event)=>{
-189            if(sidebar.classList.contains('show')){
-190             sidebar.classList.remove('show')
-191          }
-192   })
+188                                                                      
  • Take out the close sidebar icon and remove the d-sm-none class from the view map button
-58    <i title="Close Sidebar" id="modalToggle" title="Close Sidebar" class=" fa fa-times fa-3x " style="cursor: pointer; position: absolute; right: 0; margin: 1rem; color: #00000070;" aria-hidden="true"></i>
+58                                                                               
-59   <button type="button" class="btn btn-primary d-sm-none" data-bs-dismiss="offcanvas" aria-label="Close">View Map</button>
+59   <button type="button" class="btn btn-primary " data-bs-dismiss="offcanvas" aria-label="Close">View Map</button>
  • Take out the element declaration
-88  let modalToggle = document.getElementById('modalToggle')
+88                                                             
  • Finally, add some space to the open sidebar button to accommodate the geo search I used top: 30px and remove the duplicate title attribute
-53   <i title="Open Sidebar" id="mapToggle" title="Open Sidebar" class="fa fa-bars fa-3x " style="position: absolute; right: 0; margin: 1rem; z-index: 900; color: white; cursor: pointer;" aria-hidden="true"></i>
+53   <i title="Open Sidebar" id="mapToggle" class="fa fa-bars fa-3x " style="position: absolute; top: 30px; right: 0; margin: 1rem; z-index: 900; color: white; cursor: pointer;" aria-hidden="true"></i>

@jywarren I could convert this into an FTO after your review of course.

@Malik, this works! Good effort Malik. I agree that setting up event handler for the off-canvas element using the traditional approach produced the unintended effect which is the issue reported. Solely using bootstrap event handling approach cleared the issue.

Copy link
Member

Choose a reason for hiding this comment

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

Really great problem solving, and excellent suggested fix. Thanks all! @hs05june if you'd like to complete this we'd love that. But let us know and if we don't hear back in a day or two we can also add an extra commit to this PR to complete it. Thanks for your patience with a task that ended up a bit tougher than we'd expected!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made the suggested changes @jywarren . Really very thankful to you all for helping me out.

Copy link
Member

Choose a reason for hiding this comment

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

It works!!! Great work everyone!

@7malikk
Copy link
Collaborator

7malikk commented Dec 7, 2022

@hs05june Hey there, do you need any help finalizing this PR?

Signed-off-by: hs05june <harpreet.singh.cse21@itbhu.ac.in>
Signed-off-by: hs05june <harpreet.singh.cse21@itbhu.ac.in>
@jywarren jywarren merged commit d4aafae into publiclab:main Dec 8, 2022
@welcome
Copy link

welcome bot commented Dec 8, 2022

Congrats on merging your first pull request! 🙌🎉⚡️
Your code will likely be published to https://mapknitter.org in the next few days.
Now that you've completed this, you can help someone else take their first step!
Reach out to someone else working on theirs on Public Lab's code welcome page (where you'll now be featured as a recent contributor!). Thanks!

Help others take their first step

Now that you've merged your first pull request, you're the perfect person to help someone else out with this challenging first step. 🙌

https://code.publiclab.org

Try looking at this list of `first-timers-only` issues, and see if someone else is waiting for feedback, or even stuck! 😕

People often get stuck at the same steps, so you might be able to help someone get unstuck, or help lead them to some documentation that'd help. Reach out and be encouraging and friendly! 😄 🎉

Read about how to help support another newcomer here, or find other ways to offer mutual support here.

@jywarren
Copy link
Member

jywarren commented Dec 8, 2022

Great collaboration!! 🎉

@7malikk
Copy link
Collaborator

7malikk commented Dec 8, 2022

Congratulations @hs05june on merging your PR🚀🚀

Thank you @jywarren @segun-codes for your contributions🙌🏾🙌🏾

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.

Copy "location search" feature to archive.html
4 participants