ecohealthalliance / eidr

https://eidr.ecohealthalliance.org
Apache License 2.0
1 stars 0 forks source link

Map filtering #280

Closed jgoley closed 9 years ago

jgoley commented 9 years ago

Connects to #118 Connects to #289 Connects to #298 Connects to #302

jgoley commented 9 years ago

@aslagle I've made the necessary changes. Let me know what you think. I also improved the UI on mobile devices (the filtering is initially off canvas so as to not obstruct the map).

jgoley commented 9 years ago

Oh, I was wondering if this needs to be refactored so that its agnostic of the type of filter to make it easier in the future to filter by additional fields or values.

jgoley commented 9 years ago

I just noticed an issue with the menu button on small screens. Its probably a conflict caused by the styles of filter UI elements.

aslagle commented 9 years ago

About refactoring to be agnostic of the type of filter, it would be nice but we could have a separate task for that later.

jgoley commented 9 years ago

I've fixed the menu button issue but the map filtration on mobile is not great. I suppose this is not a huge priority but it needs work.

aslagle commented 9 years ago

We can have Robyn test this too.

ZachSGold commented 9 years ago

For the filters, transmission from infected animals should not appear. Instead, Transmission of the microorganism from animals to people (Event Specific) should appear in the same separated box as Zoonotic (Yes/No). Event transmission is not useful to show. If show any others they should be EID category and/or driver.

jgoley commented 9 years ago

@ZachSGold My bad. Should have looked at the issue a second time...for some reason I thought we wanted to filter by transmission. Will make this change ASAP.

ZachSGold commented 9 years ago

@jgoley Ok, no problem!

jgoley commented 9 years ago

@ZachSGold How does this look? screen shot 2015-06-23 at 2 26 12 pm

I also noticed event 71 (Escherichia coli O111:H8) has no category and, as a result, appears when is not supposed to.

ZachSGold commented 9 years ago

@jgoley Looks great! I fixed event 71 too.

aslagle commented 9 years ago

It's not clear what the search box actually searches - maybe we should make that more explicit?

Also, if you uncheck all the boxes one by one, it still says "uncheck all".

jgoley commented 9 years ago

@aslagle I added to the placeholder text: "Search by event name" I also changed how the check-all/uncheck-all toggles work. Now they appear and disappear when necessary and they can be applied to each group of checkboxes.

jgoley commented 9 years ago

I just noticed a change to the uncheck/check all isn't working properly (probably because I added it to both variables). Will fix.

aslagle commented 9 years ago

I think the checkbox logic could be cleaner if there was a separate representation of the state of the checkboxes in a variable instead of just the DOM. That state could be used to control whether "checked" is set on each and whether it says check or uncheck all, and to set the query, and it could be updated on clicks.

jgoley commented 9 years ago

Would this also apply to when the user clicks on individual checkboxes?

aslagle commented 9 years ago

Yeah I think so - the event handler for it could change the state for that individual checkbox.

jgoley commented 9 years ago

@aslagle I've updated things—moved the state of the checkboxes to a reactiveVar. I created a property called 'show' which we could use to hide the filter options for some of the variables when the page is loaded and give users the option to reveal more/collapse each variable. On large screens the UI is okay but not great on smaller screens. Should I go ahead and take care of this?

jgoley commented 9 years ago

I've updated the initial array of objects associated with the filter variables. Now there's less transformation via helpers. I still add the spreadsheetName, otherwise it would have been difficult to change the state of the checkbox when clicked.

Also, I found that 7 of the events have an EID category of 'not found'. In previous iterations I added this value in manually but there's no good way to do this now. I added it to the dropdownExplanations of eidCategory. I hope that's okay.

jgoley commented 9 years ago

I've fixed the extend issue and made changes to the un/check-all toggling.

jgoley commented 9 years ago

I've added popovers which show (on hover) the description of the variable or variable value. @ZachSGold or @robynschreiber could either of you take a look to ensure the descriptions are correct and the functionality is as desired? Thanks!

ZachSGold commented 9 years ago

@jgoley Looks good. Just needs some periods at the end of some of the explanations.

jgoley commented 9 years ago

@aslagle Should we add periods in the spreadsheet or import script considering the explanations are or could be displayed in other areas?

aslagle commented 9 years ago

Is there anywhere where it would be bad for them to have periods?

jgoley commented 9 years ago

I don't think so. Currently the only other area showing these explanations is in the variable definitions view, but its actually displaying a different column, ''webDropdownExplanations".

aslagle commented 9 years ago

Cool, then go ahead and change the spreadsheet.

jgoley commented 9 years ago

I added periods in the spreadsheet. (while doing so noticed this: #304 which is resolved in #305) @aslagle do you think this is ready to merge in?

aslagle commented 9 years ago

:+1: