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

EDSC-4125: Recurring temporal slider doesn't work #1841

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

daniel-zamora
Copy link
Contributor

@daniel-zamora daniel-zamora commented Dec 3, 2024

Overview

What is the feature?

On the granule temporal filter there are some issues that arise with the recurring toggle. If start and end have the same year, the InputRange handlers overlap and are unmoveable. When recurring is toggled on, you can still change year values with the date picker. And when you scrub the InputRange handlers left or right, on each step change it will send a search request to CMR

What is the Solution?

When the same year is selected for start and end via the datepicker, and recurring is toggled on, we max out the range from 1960 to w/e the end date is

When recurring is toggled on, the date picker will not allow you to traverse outside the bounds of a single year's worth of months(i.e. you cannot go to the previous month when on January or the next month after December) and the year is no longer displayed on the column header when in days mode. It prevents traversing by disabling the navigation buttons.

CMR searches are now only sent in the onChangeComplete callback, not onChange

What areas of the application does this impact?

Datepicker, TemporalSelection, and GranuleFilterForm components

Testing

Reproduction steps

  1. Select a Collection and filter granules by temporal filter
  2. Select a start and end with the same year
  3. Toggle recurring on.
  4. Attempt to move the InputRange handle.

Attachments

Please include relevant screenshots or files that would be helpful in reviewing and verifying this change.

Checklist

  • I have added automated tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@daniel-zamora
Copy link
Contributor Author

in draft, just running tests

Copy link

codecov bot commented Dec 3, 2024

Codecov Report

Attention: Patch coverage is 56.92308% with 56 lines in your changes missing coverage. Please review.

Project coverage is 82.41%. Comparing base (1362ddd) to head (1a9c298).

Files with missing lines Patch % Lines
...ents/TemporalDisplay/TemporalSelectionDropdown.jsx 0.00% 21 Missing and 5 partials ⚠️
...s/components/GranuleFilters/GranuleFiltersForm.jsx 25.92% 15 Missing and 5 partials ⚠️
static/src/js/components/Datepicker/Datepicker.jsx 86.66% 8 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1841       +/-   ##
===========================================
- Coverage   93.80%   82.41%   -11.39%     
===========================================
  Files         778      778               
  Lines       18888    18991      +103     
  Branches     4849     4878       +29     
===========================================
- Hits        17717    15652     -2065     
- Misses       1123     3030     +1907     
- Partials       48      309      +261     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@daniel-zamora daniel-zamora marked this pull request as ready for review December 4, 2024 13:15
@eudoroolivares2016
Copy link
Contributor

I'm seeing a behavior on this branch where on the temporal filter (the top level collection filter I mean) the recurring slider if enabled is only able to include the years so it cannot actually be moved vs on the granule filter we can still slide back to previous or post times

@daniel-zamora daniel-zamora force-pushed the EDSC-4125 branch 2 times, most recently from cb9a164 to 74a3546 Compare December 17, 2024 20:04
@bnp26
Copy link
Collaborator

bnp26 commented Dec 18, 2024

@daniel-zamora What effort level do you think converting Datepicker.jsx & TemporalSelection.jsx to a functional component would be?

@trevorlang trevorlang self-requested a review December 18, 2024 22:05
Copy link
Collaborator

@trevorlang trevorlang left a comment

Choose a reason for hiding this comment

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

Definitely seeing some nice improvement here. One thing I noticed though, if a user does not have any dates selected when checking "Recurring" they end up in the state where the start and end slider handles are overlapping and the range cannot be changed. It would be nice if instead, the entire range was selected.

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.

4 participants