added modal to the infomartion display #447
Conversation
There was a problem hiding this comment.
@neelesh17, did this work when you ran it locally? This is what I am getting when I clicked the info button:
|
yes @crisner it is working locally on my device. |
|
Awesome! Thanks! Could you fix the conflicting files? |
|
@crisner I made the changes you requested. |
src/util/displayInfo.js
Outdated
| this._infoDisplay.classList.add(this.options.classname); | ||
| this._infoModal = L.DomUtil.create('div'); | ||
| this._infoModal.classList.add('modal', 'fade', 'leaflet-bar', 'leaflet-control-info'); | ||
| $(this._infoModal).attr('role', 'dialog'); |
There was a problem hiding this comment.
Hi, this is looking really nice. I wanted to suggest you could construct the HTML as strings and it might end up a bit more readable... what do you think? For reference, see this section of another project where a block of HTML/CSS is constructed in a string:
What's more, it used standard Bootstrap classes rather than unique css styles. This could be helpful for you too because it would add standard margins, padding, etc and it might make your code overall shorter. If you use the standard Bootstrap html/css as in https://getbootstrap.com/docs/4.0/components/modal/, you may not need as many customizations of the style attribute, you know? And it may accommodate more varied content since those styles have been so thoroughly tested.
What do you think?
Thanks again for your help!!! 🎉
There was a problem hiding this comment.
Hi, I think it is a nice idea to construct the HTML as string and as for the unique css styles, i added those because the styles present in the standard bootstrap were being overwritten due to another class.
There was a problem hiding this comment.
@jywarren i made the changes you requested but the test are failing and i m not able to find the cause, would you help me out.
…mental-layers into info-display-changed
Hmm, what could be causing this error? https://travis-ci.com/github/publiclab/leaflet-environmental-layers/builds/158924297#L301 |
| else { | ||
| var infoModal = "<div class='modal fade leaflet-bar leaflet-control-info' role='dialog' tapindex='3' aria-hidden='true' >"; | ||
| } | ||
| infoModal += "<div class='modal-dialog'>"; |
There was a problem hiding this comment.
This is really nice and clean. Can you share a screenshot?
|
Hopefully this error is fixed by #454 ? |
|
Indeed, could you try rebasing and rebuilding it? Thanks! there are more tips and guides on rebasing here: https://publiclab.org/wiki/developers#Resources |
Fixes #444 (<=== Add issue number here)
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
@publiclab/reviewersfor help, in a comment belowIf tests do fail, click on the red
Xto learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Thanks!