MM-230 Use common panel to display search results and include # of co-ops filtered #246
Conversation
|
@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. |
|
I've had a look through. More specific comments to follow.
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. |
src/map-app/localisations.ts
Outdated
| errorLoading: "FIXME", | ||
| hideDirectory: "Masquer l’annuaire", | ||
| in: "dans", | ||
| loading: "Chargement des données", | ||
| mapDisclaimer: "Cette carte contient des indications sur les zones où il y a des différends au sujet territorial. L'ACI n'approuve ni n'accepte les frontières représentées sur la carte.", | ||
| matchingResults: "matching results", // TODO: translate |
There was a problem hiding this comment.
Maybe these translations be todone on this branch?
There was a problem hiding this comment.
@ColmDC What's the process for doing these translations? I think I could manage French and Spanish using Google Translate, Linguee, and my knowledge from GCSE, but I wouldn't feel confident in my Korean translations.
There was a problem hiding this comment.
We usually do a google translate and ask ICA if they wish to provide better ones.
There was a problem hiding this comment.
Can you specify the mising terms?
There was a problem hiding this comment.
These are what I have so far, using Google translate:
EN: Apply Filters
FR: Appliquer les filtres
ES: Aplicar filtros
KO: 필터 적용
EN: N directory entries
FR: N entrées d'annuaire
ES: N entradas de directorio
KO: N 디렉토리 항목
EN: N matching results
FR: N résultats correspondants
ES: N resultados coincidentes
KO: N 일치하는 결과
(I've included the N's here for context, to represent a number. The translated text will appear after a number e.g. "123 directory entries")
There was a problem hiding this comment.
Wondering about the "N"s - partly if they translate literally to other languages, partly if it's a widely understood layman's term to use in English, and partly, now I look in the diff, if it's just a mistake which isn't in the original phrase?
I suppose there's a question about where the number comes in the other language phrases, in particular Korean. I know nothing useful about Korean...
There was a problem hiding this comment.
The N isn't in the translation, it's just "directory entries". I included the N as context that the string will come after a number.
According to Google translate, Korean doesn't always put the number before the string, but I'm thinking we just ignore this and see if anyone complains? To do it properly, we would have to improve the way we do localisation by including context comments and a way to do string interpolation.
| isSelected(initiative: Initiative): boolean { | ||
| // Currently unimplemented - I can see an sea-initiative-active class | ||
| // being applied if a test was true, but it makes no sense in the current | ||
| // context AFAICT. The test was: | ||
| // initiative === this.contextStack.current()?.initiatives[0] | ||
|
|
||
| // The main thing is seemed to do is highlight an initiative (or | ||
| // initiatives) in the directory pop-out list of initiatives in a | ||
| // category. | ||
|
|
||
| // For now, just return false always. We may re-implement this later. | ||
| return false; | ||
| const selectedInitiatives = this.mapPresenter?.getSelectedInitiatives() ?? []; | ||
| return selectedInitiatives.includes(initiative); | ||
| } | ||
|
|
||
| toggleSelectInitiative(initiative: Initiative) { | ||
| // Currently unimplemented. | ||
| const selectedInitiatives = this.mapPresenter?.getSelectedInitiatives() ?? []; | ||
|
|
||
| // EventBus.Markers.needToShowLatestSelection.pub(lastContent.initiatives); | ||
| if (selectedInitiatives.includes(initiative)) { | ||
| EventBus.Markers.needToShowLatestSelection.pub(selectedInitiatives.filter(i => i !== initiative)); | ||
| } else { | ||
| EventBus.Markers.needToShowLatestSelection.pub([...selectedInitiatives, initiative]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Puzzled by this - seems to be re-implementing initiative selection, something which I had thought maybe used to be a thing, long ago but had been unused and probably broken for ages.
Doesn't seem to be related to counting and displaying visible initiatives?
Actually, a lot of the changes in this branch don't appear to be, so general puzzlement.
There was a problem hiding this comment.
This method wasn't implemented, but I needed it in order to highlight the selected initiative in the sidebar.
The previous code for populating the initiative sidebar tried to achieve this by setting the class .sea-initiative-active but this didn't work because the class didn't persist when the sidebar UI was refreshed.
There was a problem hiding this comment.
I'm uncertain, but I seem to remember that selection stuff is to do with selecting markers on the map, not selecting sidebar items.
I think here you're just using them to zoom into the marker? Via EventBus.Markers.needToShowLatestSelection?
Not a big deal, just checking one sort of functionality isn't being confused for another and revived to serve a different purpose.
There was a problem hiding this comment.
I think because I'm coming from LX, I'm seeing things in a Redux "app state" kind of way.
We do sort of keep a state of the selected markers on the map, within the currentlySelected array (see your comment below). When we click on an initiative in the sidebar, or on a marker directly, it adds the initiative to this array. So I've written this function to expose this state, so that the currently selected initiative is also highlighted in the results pane. It should work however the marker is selected.
Tangential to this point: I think this is actually the most frustrating bit of the UI. The app's state is fragmented and stored all over the code, within different classes. And then if another class wants to access or change the state, it's a nightmare to thread the information through a chain of functions, especially if they're in different parts of the UI. The EventBus broadcasts try to solve this but it basicaly violates the point of OOP, feels quite hacky, and doesn't make the code any clearer.
It's a limitation of OOP that I experienced as a Java Android developer, since it often leads to anti-patterns, unless the code is written in a very strict and methodical way with a clear structure (which this isn't).
If we were to re-write some of the UI code, I think this might actually be the most beneficial change- to move app state to a central store such as Redux, which any part of the UI can hook into. Since we implemented Redux more widely in LX, it has made the code a lot cleaner. I think this is more pressing than changing to something like React. The html structure is already fairly understandable and d3 doesn't seem terrible.
There was a problem hiding this comment.
I think something like this would be a first step to anything similar to introducing react anyway.
However, suspect it still won't be easy! There is a lot of stuff, and untangling all the state might not be simple. Therefore I suspect it would need to be further broken down into steps which can be attempted independently...
I've tried to get rid of the EventBus stuff as much as I could and at least make it typesafe (previously it was easy to send malformed messages to misnamed targets, and what the allowed targets was totally opaque and undocumented). I've not entirely succeeded but it's better than it was. What probably needs to be done is to continue with that so that we have a kind if "central source of truth" which behaves more like you get in Redux. But there are other bits of state and coupled logic all over the place, and I don't know how deep those depths go...
Which ultimately means we need to get budget-holder clearance to put our snorkels on and try investigating - but leave a route to bail out if it starts getting too tangled or taking too long.
On the other hand - we might want to look at the bigger picture an see if that goes somewhere we need to be, or if we can instead dredge out the useful bits and retrofit them into a new framework - perhaps if we want to be able to make very big maps? Therefore needs the kind of high-level outcomes planning we scheduled (for today but had to postpone.)
@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. |
wu-lee
force-pushed
the
230-initiative-filtering-ui
branch
from
9b52fe6
to
00185c1
Compare
rogup
force-pushed
the
230-initiative-filtering-ui
branch
from
00185c1
to
5ca220f
Compare
|
@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. |
|
I think it's probably just as well to pick option 1, yes. |
|
Does this need to be in the In Review column? is there anything blocking my seeing this in action? |
|
@ColmDC Please could you confirm these translations with ICA #246 (comment) Then I'll write them in and send it to you for QA |
|
Just add google translations for them and send to QA. I'm keen to close this one. |
…fo sidebar from DirectorySidebarView to a) BaseSidebarView and b) SidebarView
…ove scrollable results section
…s of BaseSidebarView to choose how much of the UI is refreshed, based on whether the panel changed
… to initial state
rogup
force-pushed
the
230-initiative-filtering-ui
branch
from
5ca220f
to
94e6249
Compare
|
The above merge looks like a fast-forward merge? i.e. it looks like the branch has been flattened into |
|
@wu-lee I clicked 'merge with rebase' on GitHub |
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.