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

[Dotcoop - Misc] Display # of co-ops filtered in UI #230

Open
ColmDC opened this issue Feb 2, 2024 · 32 comments · Fixed by #246
Open

[Dotcoop - Misc] Display # of co-ops filtered in UI #230

ColmDC opened this issue Feb 2, 2024 · 32 comments · Fixed by #246
Assignees
Labels
billable Work against a grant of for a client

Comments

@ColmDC
Copy link
Contributor

ColmDC commented Feb 2, 2024

Please track against the clockify project [Dotcoop - Misc]

Description

Implement the design chosen in #232 to display the number of results found matching the text search and filter options chosen.

Designs: https://xd.adobe.com/view/aefe2e1a-612a-4ed4-b570-0bf513927e8c-fbad/grid
Updated designs 3/4/24: https://xd.adobe.com/view/06927e2d-af74-4e6f-8749-35a116f28e33-53c1/grid

Note the significant additional change here to have the filter results open in a new panel instead of underneath, unifying the design pattern between the directory panel and the filter panel.
On mobile only one panel is displayed at a time, so if a user wishes to add more than one filter they will be switched to the new panel unexpectedly which would be a bad UX. Hence the addition of the 'Apply Filters' button to enable the user to add multiple filters on mobile.

Acceptance Criteria

  • The count should include initiatives with no geolocation, so with no marker on the map.
  • When all filters are clear and no text search set, it should display the number of all the initiatives in the directory.
  • The display method should work for between 0 and 999,999 initiatives.
  • It should work on all screen sizes.
  • It should be enabled/disabled via map configuration.
  • Palette should be configurable with other widget palettes.
@ColmDC ColmDC added billable Work against a grant of for a client blocked Can't be progressed until another Issue has been completed labels Mar 8, 2024
@lin-d-hop lin-d-hop removed the blocked Can't be progressed until another Issue has been completed label Mar 27, 2024
@ColmDC
Copy link
Contributor Author

ColmDC commented Mar 28, 2024

Be aware of #238 and #197 while implementing this Issue.

@rogup
Copy link
Contributor

rogup commented Apr 10, 2024

For the dark panel displaying filter results when using the Directory, I can see in the code that there is logic which makes the background different colours, depending on the type of initiatives (housing, energy, etc.). But this seems to be customer-specific, for a specific source of data. See am20, etc in the mykomap style.css file.

@wu-lee or @ColmDC : What does am refer to and is this colour-coding still something that we want to keep? This would complicate things because we need to use the same panel for search results, and the colours wouldn't make sense for a more complicated search. And in any case, I think it's strange that we have hardcoded customer-specific logic in the mykomap repository.

Maybe we could just colour code types of initiative randomly/sequentially to simplify things. But I think this would fall out of the scope of this ticket, and for now we could just use a black panel for everything?

@rogup
Copy link
Contributor

rogup commented Apr 10, 2024

Also, regarding re-using the black panel, I'm wondering if we need the panel to stay exactly the same for the Directory sidebar? (see below)

Screenshot 2024-04-10 at 17 06 36

I think the code could be simplified if the black panel was the same for both the Directory and Search. This would mean that the title at the top of the black panel would be removed (but we could add logic so that the selected type remains highlighted in the leftmost Directory panel). And maybe it would make sense in this case to replace the 'x' close button with 'clear selection' instead of 'clear filter' as suggested in the design?
@ms0ur1s @ColmDC do you think we could get away with these small changes, to make the panel's appearance consistent and allow the code to be a lot simpler?

@ms0ur1s
Copy link

ms0ur1s commented Apr 11, 2024

@ms0ur1s @ColmDC do you think we could get away with these small changes, to make the panel's appearance consistent and allow the code to be a lot simpler?

@rogup, for my part I think that all makes great sense, and I'd go ahead with what you suggested. What's your opinion @ColmDC?

@ColmDC
Copy link
Contributor Author

ColmDC commented Apr 12, 2024

It's hard for me to follow exactly the changes you are proposing, but

  • yes, to dropping colour coding.
  • yes to keeping search and directory panels as consistent as we can

With dropping the title 'the title at the top of the black panel ', just be conscious that it's not needed in the narrow screen.

But I think it go ahead and implement, using your best judgement.

@rogup
Copy link
Contributor

rogup commented Apr 15, 2024

@ColmDC This is almost ready for testing. A couple of things in the acceptance criteria:

It should be enabled/disabled via map configuration.

Do we definitely need this? It seems like a feature that most users would probably find useful, so could we enable it for everyone? This would keep our config options simpler.

Palette should be configurable with other widget palettes.

What does this mean?

@ColmDC
Copy link
Contributor Author

ColmDC commented Apr 16, 2024

Do we definitely need this? It seems like a feature that most users would probably find useful, so could we enable it for everyone? This would keep our config options simpler.

Agreed. Lets make it standard.

@ColmDC
Copy link
Contributor Author

ColmDC commented Apr 16, 2024

Palette should be configurable with other widget palettes.

What does this mean?

Whatever colours are required by the design should be configurable.

But I think we just using the sidebarButtonColour for the text colour is all we need to do.

@wu-lee
Copy link
Contributor

wu-lee commented Apr 16, 2024

Rohit asked:

For the dark panel displaying filter results when using the Directory, I can see in the code that there is logic which makes the background different colours, depending on the type of initiatives (housing, energy, etc.). But this seems to be customer-specific, for a specific source of data. See am20, etc in the mykomap style.css file.

@wu-lee or @ColmDC : What does am refer to and is this colour-coding still something that we want to keep? This would complicate things because we need to use the same panel for search results, and the colours wouldn't make sense for a more complicated search. And in any case, I think it's strange that we have hardcoded customer-specific logic in the mykomap repository.

am is for "Activities Modified" and refers to the linked data vocabulary with the same name.

SEA map, as it was then, was hard-wired to use this vocab for colouring the directory, the marker pins, and possibly other places. This means if anything else is used, the colour used is black. A known problem, but one which hasn't been fully fixed to date, probably just because of time and priorities. I have some ideas for how to fix.

Hunting for relevant issues:

Maybe we could just colour code types of initiative randomly/sequentially to simplify things. But I think this would fall out of the scope of this ticket, and for now we could just use a black panel for everything?

I think it's out of scope. Fixing this needs considering all the various parts: markers, directory, custom styles thereof and the possibility of various vocabs (or none) being used for the initiative's (AKA organisation, marker, pin) attribute in question.

@wu-lee
Copy link
Contributor

wu-lee commented Apr 16, 2024

Do we definitely need this? It seems like a feature that most users would probably find useful, so could we enable it for everyone? This would keep our config options simpler.

Agreed. Lets make it standard.

Be aware that making it standard means that maps which don't have this now will at some point suddenly start having it, maybe to the surprise of the customer using it. Would they care? I don't know, but they might. I think you're not proposing this would be unconfigurable - if not, we could then turn it off in advance, but it would need to be something we're aware of whenever deploying upgrades. For this reason I'd slightly be in favour of making this backward compatible by default, at least until the next major release.

@ColmDC
Copy link
Contributor Author

ColmDC commented Apr 16, 2024

Be aware that making it standard means that maps which don't have this now will at some point suddenly start having it, maybe to the surprise of the customer using it. Would they care? I don't know, but they might. I think you're not proposing this would be unconfigurable - if not, we could then turn it off in advance, but it would need to be something we're aware of whenever deploying upgrades. For this reason I'd slightly be in favour of making this backward compatible by default, at least until the next major release.

At @rogup 's suggestion I was happy for it not to be configurable. I can't imagine it being a problem for anyone. If it is, we could make it configurable then?

@rogup
Copy link
Contributor

rogup commented Apr 16, 2024

@wu-lee I think this relates to what you were saying in the All Hands about standardising Mykomaps so we don't have so many different client-specific versions to develop, test, and maintain.
The fewer config options we have, the less things that can go wrong, and the less work there is for us.

So my opinion is to always try to avoid adding config options, unless it's something a client is very likely to not accept. And maybe in these cases, we should have a process where the feature is enabled for everyone and then, if nobody complains about it, the config options is removed altogether in the next release.

@wu-lee
Copy link
Contributor

wu-lee commented Apr 16, 2024

I'm thinking of the ICA, I get the impression they're quite particular about how the map looks based on their feedback.

@ColmDC - this is more your call than mine.

I confess I've not seen the changes this implements. So this specific case might be arguable. And I agree that making things simpler to deploy is an aim.

But on the general principle "always avoid adding more config options" - it seems untrue to say that a map which is hardwired to one feature set is always a win just because it reduces configuration/implementation effort. A "reductio ad adsurdum" argument seems to be possible here.

Using the same principle of "simpler is better" could be used to argue that a better solution than not implementing the config would be not implementing this entire feature. But the reason we are is that it was asked for by DotCoop - for their specific use-case.

We can't always anticipate what clients will accept, and they've all got their own quirky and subjective opinions. Forcing an arbitrary feature which client A asked for onto client B, C, and D etc. seems like a risky proposition.

WRT to reducing complexity and increasing our agility delivering maps: a major source of our difficulties in managing variations between maps comes from the need to deal with different data sets, with different fields, which need to be handled differently. And showing this in the pop-up, and all the styling on top of that. We can't really get rid of that configuration - and that sort of configuration totally dwarfs the above sort of configuration. The need to do this configuration is the primary reason why, as Lynne put it "Creating a new map requires a bunch of meetings and a decent amount of dev".

In terms of standardisation, what I was suggesting is to tackling that aspect. And probably at the application level (i.e. build a "Mykomap project to end all Mykomap projects" - for a defined set of cases which cover what we need.)

Sorry for being so verbose.

@ColmDC
Copy link
Contributor Author

ColmDC commented Apr 17, 2024

To close this out, let's have it NOT configurable. It is not one of our complicated configuration issues.

@ColmDC ColmDC changed the title Display # of co-ops filtered in UI [Dotcoop - Misc] Display # of co-ops filtered in UI Apr 19, 2024
@ColmDC
Copy link
Contributor Author

ColmDC commented May 8, 2024

What's blocking this moving into QA?

@ColmDC
Copy link
Contributor Author

ColmDC commented May 8, 2024

Does the Clear Filter button now reset the filters to the startup configurations or reset them all to empty (text box) and -Any- (filters)?

@rogup
Copy link
Contributor

rogup commented May 10, 2024

@ColmDC 'Clear filters' resets all the filters to -Any- and empties search box. What do you mean by the startup configurations?

@ColmDC
Copy link
Contributor Author

ColmDC commented May 10, 2024

Clear filters' resets all the filters to -Any-

Good. That is what is expected. I just noticed that since the ability to configure filters to be set on map startup, Clear filters resets the filters to the startup configuration, and doesn't clear them. See https://www.workers.coop/map/.

@rogup
Copy link
Contributor

rogup commented May 10, 2024

@ColmDC This is now ready for QA, available here https://dev.maps.solidarityeconomy.coop/qa

@ColmDC
Copy link
Contributor Author

ColmDC commented May 12, 2024

Cool. @ms0ur1s could you take a look and see if it behaves as you expect?

@ms0ur1s
Copy link

ms0ur1s commented May 13, 2024

Cool. @ms0ur1s could you take a look and see if it behaves as you expect?

No worries @ColmDC, will give a once over now.

@ms0ur1s
Copy link

ms0ur1s commented May 13, 2024

@ColmDC and @rogup all looking good.

Only problem I noticed was on the mobile view. The close button is no longer working on the info panel for a selected, filtered result. So that once the panel is opened there is no way of navigating back to the search results other than by refreshing.

Steps to recreate:

  • Click on search icon
  • Select your search criteria
  • Click Apply Filters
  • Select a result
  • Click on close icon at the top right of the panel

Tested using dev tools, my iphone 6s and across multiple devices using an in-browser mobile simulator extension (Mobile FIRST - definitely not infallible, but generally dependable).


image

@ColmDC
Copy link
Contributor Author

ColmDC commented May 13, 2024

I see that too.

What did we decide about the Apply Filters button? Were we going to wait until it was clicked before applying filters and search text matching? Were we going to do it in Mobile and Large screen? It seems the button disappears in Large screen mode?

I'm also seeing with the ICA example, the countries are not displayed when you first run it up. They appear if you click Directory icon. (The bug is not exibited in DotCoop map.)

@rogup shall I write these up as fresh issues?

@ms0ur1s
Copy link

ms0ur1s commented May 13, 2024

What did we decide about the Apply Filters button? Were we going to wait until it was clicked before applying filters and search text matching? Were we going to do it in Mobile and Large screen? It seems the button disappears in Large screen mode?

I think this functionality is fine, as the Apply Filters button is unnecessary on large screens. It all reduces the amount of actions necessary before results are returned.

@rogup
Copy link
Contributor

rogup commented May 14, 2024

I think the Apply Filters button behaviour matches the spec that was decided.

I'm happy to fix those 2 regressions within this issue @ColmDC since they were caused by these code changes. Unless you would prefer to make new issues. The regressions are:

  1. The close button not working on the initiative info sidebar when in mobile mode
  2. The individual directory entries not appearing when some maps (e.g. ICA) are first loaded, probably due to a race condition

@ColmDC
Copy link
Contributor Author

ColmDC commented May 15, 2024

Please do see if these regressions are quick to fix. If not then let's create issues for them.

@ColmDC
Copy link
Contributor Author

ColmDC commented May 15, 2024

I think at some point we had decided that the apply filters button would be on all screen sizes and that the filtering would not be triggered until the button was pressed, but I can see that this didn't make it across into this ticket. Would it be straight forward to make those changes?

@ColmDC
Copy link
Contributor Author

ColmDC commented May 16, 2024

Let's just fix the regressions. Add furhter chnages would require new issue.

@ColmDC
Copy link
Contributor Author

ColmDC commented May 24, 2024

Just checking that I'm not a block on this one.

@rogup
Copy link
Contributor

rogup commented May 28, 2024

No, I've been in the middle of other work so haven't got to this yet, but planning to finish it this week.

@rogup
Copy link
Contributor

rogup commented Jun 4, 2024

@ColmDC The regressions are now fixed and the new maps are deployed to https://dev.maps.solidarityeconomy.coop/qa for testing

@ColmDC
Copy link
Contributor Author

ColmDC commented Jun 5, 2024

Looking good. Behaves well on mobile now too. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
billable Work against a grant of for a client
Projects
None yet
5 participants