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

fix: ensure latest tab stays expanded when editing a point or polygon on the map in MapAndLabel #3753

Merged
merged 3 commits into from
Oct 3, 2024

Conversation

jessicamcinchak
Copy link
Member

Changes:

  • Fixes issue where tabs were collapsing when editing a point or polygon on the map by adding a setActiveIndex() call inside of handleGeojsonChange (will keep latest tab expanded, tab isn't necessarily aware of exactly which point on the map you're editing)
  • Renames editFeature to editFeatureInForm for clarity (because this does not refer to "editing" the map!)

@jessicamcinchak jessicamcinchak changed the title fix: ensure latest tab stays expanded when editing a point or polygon in MapAndLabel fix: ensure latest tab stays expanded when editing a point or polygon on the map in MapAndLabel Oct 2, 2024
@jessicamcinchak jessicamcinchak requested a review from a team October 2, 2024 13:26
Copy link

github-actions bot commented Oct 2, 2024

Removed vultr server and associated DNS entries

Copy link
Contributor

@RODO94 RODO94 left a 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);
Copy link
Contributor

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?

Copy link
Contributor

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);
} 

Copy link
Member Author

@jessicamcinchak jessicamcinchak Oct 3, 2024

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

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

@jessicamcinchak jessicamcinchak Oct 3, 2024

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" !)

Copy link
Contributor

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.

Copy link
Member Author

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);
Copy link
Contributor

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);
} 

Copy link
Contributor

@DafyddLlyr DafyddLlyr left a 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 👍

@jessicamcinchak jessicamcinchak merged commit fa49d8e into main Oct 3, 2024
12 checks passed
@jessicamcinchak jessicamcinchak deleted the jess/keep-tabs-expanded-on-geojson-change branch October 3, 2024 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants