-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
🔨 remove availableEntities from SelectionArray #4591
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Quick links (staging server):
Login:
SVG tester:Number of differences (default views): 0 ✅ Edited: 2025-02-26 11:35:03 UTC |
403876a
to
72228a4
Compare
29068a0
to
c105206
Compare
86cbd16
to
bc428b6
Compare
c105206
to
f7c60d4
Compare
bc428b6
to
29ef92e
Compare
f7c60d4
to
aff2037
Compare
aff2037
to
dfe1702
Compare
29ef92e
to
33f00fe
Compare
dfe1702
to
5b1b261
Compare
5b1b261
to
ce90328
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice cleanup!
I tested this
- on a standalone grapher page ✅
- on a country page with the global entity selector ✅
- on an explorer page ✅
- but for the explorer, I noticed that the previous change to the construction of
availableEntityNames
actually changed the behavior, and that the old behavior had some merit there. I'll put up a PR in which I explain this further: enhance(explorer): use append-onlyavailableEntityNames
#4610
- but for the explorer, I noticed that the previous change to the construction of
ce90328
to
f15161a
Compare
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
I tripped over the
availableEntities
of Grapher's selection in the last PR and wondered if it makes sense for the SelectionArray to manage both, the selected and the available entities.I don't have a strong opinion but I am in slight favour of removing the available entities from the SelectionArray. I don't see a compelling reason for them to be part of the SelectionArray interface, and the fact that the available entities need to be kept up-to-date is a potential foot gun.
I also wonder if we could unify SelectionArray and FocusArray now that both essentially do the same thing (manage a selection).
Feel free to stop me – again, no strong opinion from my side.