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

Enhance mosaic selector #88

Merged
merged 24 commits into from
Feb 2, 2024
Merged

Enhance mosaic selector #88

merged 24 commits into from
Feb 2, 2024

Conversation

vgeorge
Copy link
Member

@vgeorge vgeorge commented Jan 30, 2024

This contributes to #85. In this PR I intent to connect the mosaic selector modal to the Planetary Computer API in order to fetch mosaic presets and temporal extends of collections. This is WIP, opening for visibility.

Current changes:

  • Removed existing mosaics tabs
  • Introduced SWR module (see Adopt common request library #87)
  • Created a custom hook to fetch mosaic presets and collection metadata from PC
  • Populated select input with mosaic options

To do:

  • Auto-select the first mosaic preset from PC
  • Update mosaic map when a preset is changed
  • If a custom date is selected, set selector option to 'Custom date'
  • Set min/max date of custom date selector using collection metadata from PC
  • Improve loading state as PC will be queried when the component is mounted

@LanesGood Now I'm wondering if using mosaic presets from PC is a good approach. The first preset is 'Most recent (low cloud)'. On Pearl, this label will be displayed for exported predictions maps that might have been generated over one year ago, causing an inaccuracy. We might need to generate the season presets in UI, using the temporal extend of the collection provided by PC STAC API.

@LanesGood
Copy link
Member

@vgeorge good point on the PC "Most recent" label. Would it be possible to grab every mosaic name except for the latest, and generate a new name for that one?

@vgeorge vgeorge marked this pull request as draft January 31, 2024 10:18
@vgeorge vgeorge marked this pull request as ready for review January 31, 2024 13:18
@vgeorge
Copy link
Member Author

vgeorge commented Jan 31, 2024

@LanesGood the 'Most recent' preset comes from PC data API, not the PC STAC API. They are hardcoded values generated by PC's team and we don't have any control over it. While 'most recent' preset exist for both Sentinel 2 and NAIP, there are other collections that do not follow this scheme. A more future-proof approach is to use the acquisition start and end, which is part of the STAC API to generate our quartely presets. This is how I implemented now.

This is ready for review. You should be able to:

  • Open modal and see the latest quarter selected by default
  • Change quarter presets and see the date inputs and map preview updated
  • Change date inputs and see 'Custom date' option selected and map preview updated
  • Submit the selected mosaic, which will close the modal and update the base map

Additional notes:

  • The styles are not exactly as in Figma, it would be great if you could update them
  • If a mosaic is already selected in the project, it is not set as the default value when the modal is opened. I believe this behaviour is ok because once we implement the timeframe switch (like the one from checkpoints, as discussed), the modal would be open only a new mosaic is about to be added
  • Cypress specs are failing because mosaic card have been removed. I'll look into fixing those in parallel.

Please let me know if you have any questions.

Copy link
Member

@LanesGood LanesGood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vgeorge looking good! I've pushed up some changes for the styles.

Two items I noticed that would be best to change:

The Mosaic name is still set to the long name, even though the modal displays the short name. Ex, I picked Dec - Feb 2023 in the modal, but the panel then says Sentinel 2 Level-2A 2023-12-27 11:25UTC...

Also it looks like I can change the end date after setting the start date. I can even set an end date prior to my start date! Do we still want to limit this to just 90 days, and have a disabled end date?

@vgeorge
Copy link
Member Author

vgeorge commented Feb 2, 2024

@LanesGood thanks for the review and the styles updates. I added the following changes:

  • Add form validation to check if start date is at least 1 day before end date
  • Fix cypress spec
  • Fix API call by retrieve existing mosaic if creation failed because of existing id
  • Improved mosaic date range label, I opted for displaying the day of the month all times for simplicity
  • Set min-height to loading wrapper to keep modal height consistent when loading

Can you please do another review?

@LanesGood LanesGood linked an issue Feb 2, 2024 that may be closed by this pull request
7 tasks
Copy link
Member

@LanesGood LanesGood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vgeorge looking good! I like the minimum height setting on the modal

I do notice that the minimum 24 hours difference between mosaic start and end means that I need to have an end date of at least 2 days later. This makes sense if I think about it, but should we change the text to "end date must be 2 days..."?
image

Two other non-blocking things I note that could be enhancements:

  • add helper text explaining why a user might not see anything in the mosaic (if very dark, not enough scenes, and if blank, not long enough period for image collection, etc)
  • explore keeping the separator between mosaics as a dash - to match mosaic selection

Finally, I made a minor style change to group start + end date in one line.
image

@vgeorge
Copy link
Member Author

vgeorge commented Feb 2, 2024

@LanesGood thanks for the updates, it looks great. I agree with your suggestions and updated added the first and the third. Let's track the remaining as an enhancement.

@vgeorge vgeorge merged commit 35dda4e into develop Feb 2, 2024
3 checks passed
@vgeorge vgeorge deleted the enhance/mosaic-selector branch February 2, 2024 17:23
@vgeorge vgeorge mentioned this pull request Feb 5, 2024
4 tasks
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.

Mosaic creation + preset updates
2 participants