DigitalCommons / mykomap

A web application for mapping initiatives in the Solidarity Economy
3 stars 0 forks source link

MM-230 Use common panel to display search results and include # of co-ops filtered #246

Closed rogup closed 2 months ago

rogup commented 3 months ago

Closes #230

The main change here is to generalise the dark initiatives list panel used in DirectorySidebarView, to also be used to display the list of initiatives that match a search. In that process of moving code over to the abstract class, I also spotted a lot of unnecessary duplicated code in the child classes, which I removed.

This code to create the initiatives list panel has been moved to BaseSidebarView, and a count of results has been added to its header. It should appear whenever a search is performed or a directory item is clicked.

For smaller screens with a width of < 1080 pixels, the panel only displays once 'apply filters' has been clicked, since it appears in front of the sidebar. The logic and css for detecting smaller screens has been fixed up since it was a bit inconsistent.

rogup commented 3 months ago

@wu-lee Please could you review these changes? The UI code is a mess so I think it will be difficult. It was hard to separate my changes into separate commits due the messiness of the code, where I kept discovering different code paths during development.

wu-lee commented 2 months ago

I've had a look through. More specific comments to follow.

The UI code is a mess so I think it will be difficult. It was hard to separate my changes into separate commits due the messiness of the code, where I kept discovering different code paths during development.

Yes indeed. Welcome to my world.

As someone who doesn't often get asked to review other people's code, I'm wondering about the following options:

It's a bit of a dilemma. I might go with the first option, but with a dash of two, after a break for a hot cup of tea; mitigate this with an attempt to defuse tension through humour; then if I'm feeling ambitious perhaps have a crack at four, but with a reserve option of five or six if all this works out badly.

rogup commented 2 months ago

As someone who doesn't often get asked to review other people's code, I'm wondering about the following options:

  • hide my befuddled expression with a thumbs-up and wave it on through
  • work through the detailed changes and ask lots of perplexed and borderline passive aggressive questions in comments
  • request that you rebase it splitting it out into the following categories I can discern

    • cosmetic changes (indentation, spelling, bracketing, wrapping)
    • symbol name changes
    • removal of old/duplicate code
    • changing the implementation of apparently unrelated functionality
    • a new "apply filters" button
    • changes to the way search results are displayed on small screens
    • the addition of the search result total
  • rebase it myself to the same end and flex on you with my git tips and tricks
  • defer replying until you give up and merge it yourself
  • give up and learn to farm

@wu-lee I can have a go at rebasing the code to make it easier for you. I think this would be be best practise, but trying to do so felt daunting because the code was quite messy so it would take a while for me to untangle the commits, and I don't know if this was worth the effort.

But I think it would be good for me to get more confident in rebasing anyway, and I'll need to do it for the PR, unless I do one massive merge. Could we have a call where you show me how you do rebasing?

wu-lee commented 2 months ago

@wu-lee I can have a go at rebasing the code to make it easier for you. I think this would be be best practise, but trying to do so felt daunting because the code was quite messy so it would take a while for me to untangle the commits, and I don't know if this was worth the effort.

But I think it would be good for me to get more confident in rebasing anyway, and I'll need to do it for the PR, unless I do one massive merge. Could we have a call where you show me how you do rebasing?

Sure - we can compare methods. I'm a bit sketchy about how to do it with VSCode/GitLens, mostly I do it with Emacs/Magit/CLI, but would be interested in working out how to.

rogup commented 2 months ago

@wu-lee I've rebased the commits as best as I could, but there's still probably a bit of messiness unless I spent many more hours.

So I think all that's left, unless you have any more review comments, is to wait on confirmation about the translations. Then I'll send it over to Colm for QA.

wu-lee commented 2 months ago

I think it's probably just as well to pick option 1, yes.

ColmDC commented 2 months ago

Does this need to be in the In Review column?

is there anything blocking my seeing this in action?

rogup commented 2 months ago

@ColmDC Please could you confirm these translations with ICA https://github.com/DigitalCommons/mykomap/pull/246#discussion_r1583450832

Then I'll write them in and send it to you for QA

ColmDC commented 2 months ago

Just add google translations for them and send to QA. I'm keen to close this one.

wu-lee commented 2 months ago

The above merge looks like a fast-forward merge? i.e. it looks like the branch has been flattened into dev.. compare with 230-initiative-filtering-ui on github currently which has identical commits.

rogup commented 2 months ago

@wu-lee I clicked 'merge with rebase' on GitHub