-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix: ensure latest tab stays expanded when editing a point or polygon on the map in MapAndLabel #3753
fix: ensure latest tab stays expanded when editing a point or polygon on the map in MapAndLabel #3753
Conversation
Removed vultr server and associated DNS entries |
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.
I had an issue when testing where if I made two points, with Tab1 and Tab2.
If I was on Tab1 and moved Point1, it auto selected Tab2
Screen.Recording.2024-10-02.at.16.03.52.mov
It may be down to the Active Index being set as the last item in the array when a feature change happens.
|
||
if (event.detail["EPSG:3857"].features) { | ||
// handleGeoJSONChange is triggered repeatedly when editing a feature on the map (eg dragging it); ensure latest tab stays active/expanded | ||
setActiveIndex(event.detail["EPSG:3857"].features.length - 1); |
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.
Should the activeIndex
relate to the index of the feature being changed rather than the last item in the array?
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.
Similar to the above - also hit this issue when testing.
Is there a way we could identify this as an "edit" interaction as opposed to an "add" interaction (maybe checking length of event.detail["EPSG:3857"].features
and formik.values.schemaData
?), and use this to control the flow here?
if (isAddingFeatures) {
setActiveIndex(activeIndex + 1);
}
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.
Thanks both - I had noted this limitation in the PR description because it's basically really hard / I'm not positive it's something that is reliably & currently dispatched by the web component when editing a point 🙃
Here's what's happening:
- When you edit/drag a point on the map, all features (the entire geojson) are dispatched to
handleGeojsonChange
- not only the single point you've dragged - therefore checking length alone isn't really meaningful (it hasn't changed!) - I think the order of features should have changed so that whichever was most recently modified is last in the list - but I'm not entirely sure how reliable this is
- If this is reliable, then we should be able to get the
label
property of the last feature and map it to it's corresponding tab index - I'm testing this now, stay tuned - If this isn't proving reliable, then I think we might need to be temporarily satisfied with tabs staying expanded at all and not minimising, irrespective of which tab
- If this is reliable, then we should be able to get the
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.
When adding a new feature, isn't there an additional item in event.detail["EPSG:3857"].features
compared to formik.values.schemaData
that could signify that this is an "add" event? And from this we can decided to update the index, or not update the active index.
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.
It might be that I'm misunderstanding the requirement here sorry!
I think it's fine to not keep a relationship between the map and active tabs, by just not updating the activeIndex on map edit. However, we can still fix the collapsing bug by just not calling setActiveIndex()
on a map edit.
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.
I think we might be getting two things confused:
- This PR is only addressing cases of editing/modifying a point on the map when a tab index can then fall out of sync currently
- Adding points on the map should always be corresponding to their correct tab already? (Adding to the map does indeed mean we can then consistently use the length of features => length corresponds to active tab because added point will be "newest/latest" - which is how it's handled in the separate context methods for adding versus "handling change" !)
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.
Yeah I think this is it sorry! 👍
I think the real difference here is I'm suggesting that we aim for the following behaviour -
- On add, set active index to final (new) item - this works as-is without issue currently ✅
- On edit, don't update active index at all - this will stop it getting out of sync (tabs collapsing as activeIndex > features.length). It does mean there's no relationship between map edits and form edits, but this is currently the case anyway.
The approach here works to solve the out of sync issue, but I think updating the index all the time to the final item is a potentially unexpected behaviour.
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.
Pizza rebuilding now with latest change if you can both try re-testing please!
|
||
if (event.detail["EPSG:3857"].features) { | ||
// handleGeoJSONChange is triggered repeatedly when editing a feature on the map (eg dragging it); ensure latest tab stays active/expanded | ||
setActiveIndex(event.detail["EPSG:3857"].features.length - 1); |
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.
Similar to the above - also hit this issue when testing.
Is there a way we could identify this as an "edit" interaction as opposed to an "add" interaction (maybe checking length of event.detail["EPSG:3857"].features
and formik.values.schemaData
?), and use this to control the flow here?
if (isAddingFeatures) {
setActiveIndex(activeIndex + 1);
}
…ss/keep-tabs-expanded-on-geojson-change
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.
Perfect! Great observation that the final item in the array is the last modified - played with it a fair bit and works really well 👍
Changes:
setActiveIndex()
call inside ofhandleGeojsonChange
(will keep latest tab expanded, tab isn't necessarily aware of exactly which point on the map you're editing)editFeature
toeditFeatureInForm
for clarity (because this does not refer to "editing" the map!)