- 
                Notifications
    You must be signed in to change notification settings 
- Fork 20
feat(#305): support select one from map #491
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
Conversation
| 🦋 Changeset detectedLatest commit: b4acdd2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
 Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR | 
| currentState.value = STATES.LOADING; | ||
| try { | ||
| // ToDo: this is cached and retry doesn't work. Cannot add cache bust parameter. | 
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.
@garethbowen, a question for when you have time later :)
One requirement is to load the map bundle only if maps are present in the form. If the bundle fails to download for some weird reason, a retry button is available. The browser doesn't automatically retry downloads unless a parameter is added to make it unique each time. However, this approach doesn't work when using WF as a dependency in Central, it can't match the bundle file name.
We decided to leave the retry feature for later.
I'm curious if you know any other way?
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 if we had control of the request we could do it, by setting the cacheMode to reload it could work but then you'd have to worry about loading the module manually. Probably not a good idea...
- The cache busting should work. I would call this a bug in central and fix it there. I assume they're being too strict in their router.
- The "it fails to download for some weird reason" condition also applies to all our other assets, right? The main JS bundle might fail to load and that would force a refresh (we don't auto retry the main bundle for example). Because we don't yet support offline maybe forcing a browser refresh is ok for now.
- Ultimately this will be fixed by going offline first and the service worker updating the asset cache for us and retrying for us.
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 didn't look at everything closely, but I thought I'd leave a few comments (all small things). The comments are just my two cents — I certainly don't want to be blocking merge or anything. I'm going to be busy tomorrow and out on Thursday, so I might not be able to reply to follow-up comments until Friday.
I need to update my Central PR to match this PR in a few places, e.g., map-styles.ts. I'll do that soon.
| ); | ||
| }; | ||
|  | ||
| const centerFeatureLocation = (feature: Feature<GeometryType>): void => { | 
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.
@matthew-white, this function now has animation included in case you need it in Central as well. In WF, when the popup is open, we need to display the feature next to the popup so it isn't covered. That's why we use some calculations with an offset instead of showing it at the map's center.
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.
Thank you! This will definitely be useful once I start working on getodk/central#1402.
…options action button.
| @garethbowen @matthew-white thanks so much for all the feedback, I have resolved them all. If you have a chance, you can take another look. I will now focus on test coverage and documentation. Tomorrow, I have a walkthrough session with QA about this feature, and I will enable the test server for them to start testing. | 
| // Prevent map cloning at low zoom during panning, which disrupts feature selection. | ||
| multiWorld: false, | ||
| projection: DEFAULT_VIEW_PROJECTION, | ||
| extent: getProjection(DEFAULT_VIEW_PROJECTION)?.getExtent(), | 
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.
@matthew-white You will need to add these lines
@srujner found an issue (video) where the selection of features was disrupted when panning clone the map. You can see the fix in my test server.
| 
 I haven't taken another look since my last review, but from my limited vantage, the PR seems to be in great shape. I'll let you know if I notice anything else or if I make a change in Central that I think should be copied in Web Forms. In the meantime, I'll leave it to @garethbowen to do the final review and approve for merge once ready. | 
| @garethbowen The QA work has finished (report), we won't be doing more changes to this PR, and it's ready for a final review 🤩 | 
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.
Awesome!
Minor suggestions inline but nothing blocking. Merge when ready!
Closes #305
Closes #258
Related PRs:
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
Test evidence
Edit submission in Central
edit-submission.mp4
Empty external files
Missing external files
Map dynamic bundle
Loads map bundle when form has a map question
It doesn't load map bundle when form doesn't have map question
Show error when it cannot load bundle
bundle-error-ui.mp4
Map control buttons
map-controls.mp4
Renders special characters
Why is this the best possible solution? Were any other approaches considered?
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Do we need any specific form for testing your changes? If so, please attach one.
Test plan
What's changed