test date input for time slider and adjust input element #1095

moontrip · 2024-05-15T12:25:07Z

This is related to #775. Tested controlled Brush by adding date input fields and it works fine: date picker is also added. In addition, considering the size of time slider, especially low height, a new prop, inputHeight, is added to adjust the height of the input element.

These are tested with storybook and video capture is made in the following. If this is alright then it will be applied to SAM.

TimeSlider.with.Date.input.and.picker.mp4

moontrip · 2024-05-21T18:40:19Z

@bobular applied to SAM and adjusted layout. It seems to work fine for me. Here are screenshots:

moontrip · 2024-05-29T02:34:49Z

@bobular step buttons are added. Tested with storybook and SAM. Some notes are:

currently it is set to be 1 year as a step
right arrow increases range.end and left arrow decreases range.start for now
considered data min/max not to go over
also added a function parameter to change number of years for step (default = 1 year)
mouseover tooltip for each arrow is added

screenshots are:

enabled
disabled

bobular

Hi @moontrip - this looks great!

I started looking at the components story and wasn't convinced - lots of things seemed broken or not yet implemented.

But then I tried the Map and it looks really cool. I love the layout.

I haven't had a look at the code yet, but I have worked on this ticket: #1103

The relevance is as follows:

the ➡️ button should move both start and end to the right by (end-start) - in other words move the selection across one "window"
obvs ⬅️ does similar
if there is not enough room to move the selection left or right then that button should be greyed out - it should not move partially into the remaining space.
it's essential that stepping back to a previous position creates exactly the same time filter (with no drift due to rounding, etc)

The last two requirements will enable stepping through the timeline to behave approximately like an animation, especially when all the requests have been cached.

My new ticket will add caching to visualization data requests also, so if the user has a floating plot on the screen, those will update quickly once all the responses have been cached.

moontrip · 2024-05-31T14:49:45Z

@bobular Thank you for your reviews and comments. Yes some features are not implemented at storybook as there are some diff between storybook and actual one at SAM.

I will change the behavior of arrows next week as you suggested: I was also thinking of "window" :) As for the 4th item, I didn't notice a specific drift when playing with those buttons. Will see/test more when addressing your feedbacks.

One thing I am concerned is that for the example case I used (screenshots), the data range is over 100 years so the step of 1 year may be too small in such a case. Perhaps we can consider a sort of a dynamic step, e.g., data range/10? This may make user a confusion, though. On the other hand, 1 year may be okay as we also have date input/picker.

bobular · 2024-05-31T14:55:13Z

Hi @moontrip - is there a mock-up that says the step will be one year? I suggest it should be whatever the size of the selection is.

Thus after one click on ➡️ the window would move like this:

   [-------]
            [-------]

moontrip · 2024-05-31T14:57:52Z

@bobular Aha I see. Thank you for your clarification. 👍

moontrip · 2024-06-04T20:17:19Z

@bobular I addressed your feedbacks and committed them. Here is a short video capture, just in case: you can test it at your local more extensively :)

TimeSlider.step.buttons.mp4

moontrip · 2024-07-01T13:09:50Z

@bobular You might have missed this review as you were hectic to deal with others. Can you please take a look at this when you are available?

bobular

This works really nicely and the EDA/MapVEu code looks good :-)

If possible please could you

look into why the left/right arrows only get disabled after a "failed click" - is it possible to pre-empt this?
When setting a year-based range (e.g. 2005-01-01 to 2006-01-01) would it be possible to handle leap-years so that the ranges always stay on Jan 1st? Even if the code is a bit ugly I think this would be worthwhile.

bobular · 2024-07-23T09:28:15Z

packages/libs/components/src/stories/plotControls/TimeSlider.stories.tsx

+    [setSelectedRange]
+  );
+
+  const handleArrowClick = useCallback(


Ah, this is STORY code. I didn't understand how it worked but now I realise I should ignore it.

moontrip · 2024-07-23T13:22:19Z

This works really nicely and the EDA/MapVEu code looks good :-)

If possible please could you

look into why the left/right arrows only get disabled after a "failed click" - is it possible to pre-empt this?

When setting a year-based range (e.g. 2005-01-01 to 2006-01-01) would it be possible to handle leap-years so that the ranges always stay on Jan 1st? Even if the code is a bit ugly I think this would be worthwhile.

@bobular Thank you for your reviews and suggestions 👍 Those suggestions seem to be quite tricky but I will see if I can address them 🤞

moontrip · 2024-07-24T13:08:25Z

@bobular Your suggestions are addressed in some ways. For the first one, overall logic was changed to handle enabled/disabled, so it is now pre-emptive. For the second one that considers leap year, there are several edge cases to deal with, I suppose. Thus, what I did is to have a toggle button to move the range by year difference (please see a screenshot below) by ignoring month/date. For example when the toggle is on, a) 2007-01-01 to 2008-01-01 -> 2008-01-01 to 2009-01-01 (year 2008 is leap year so this becomes to 2008-12-31 if the toggle is off); b) 2007-01-01 to 2007-12-31 -> 2008-01-01 to 2008-12-31 (year difference is set to be 1 year at minimum)

perhaps the toggle label needs to be changed like Year range at least which needs some space arrangement for layout. I didn't do that yet because I want to see if this approach is alright.

By the way, vectorbase backend does not respond with map data but at least timeslider can be used for testing. It would be great if you can test it and share your thoughts.

bobular · 2024-07-30T08:52:36Z

Hi @moontrip - as I mentioned in Slack, I'm not too keen on the year-based-movement-toggle. One of the reasons is that we don't offer month- or week-based movement also (which I guess we could, but it would complicate the UI and implementation more than necessary IMO). Another is that it changes the stepping behaviour of the selected range in a way that's not very clear to users.

I had an idea while walking the dogs... It solves the issue of cleanly stepping through the years if the user has chosen Jan 1 as start and end dates (or any other identical start/end days of the year), without an extra toggle switch, and without explicitly checking to see if the user entered the same day-of-the-year as start and end dates.

Firstly we need a countLeapDays in date range function. Here's one that ChatGPT provided. It looks solid, but of course double-check.

function isLeapYear(year) {
    return (year % 4 === 0 && year % 100 !== 0) || (year % 400 === 0);
}

function countLeapDays(startDate, endDate) {
    const startYear = startDate.getFullYear();
    const endYear = endDate.getFullYear();
    let leapDayCount = 0;

    for (let year = startYear; year <= endYear; year++) {
        if (isLeapYear(year)) {
            const leapDay = new Date(year, 1, 29);
            if (leapDay >= startDate && leapDay <= endDate) {
                leapDayCount++;
            }
        }
    }

    return leapDayCount;
}

Then the main idea is to count the number of leap days in the current and next ranges and calculate the difference

  const deltaLeapDays = countLeapDays(newSelectedRange) - countLeapDays(selectedRange);

Then adjust the newSelectedRange.end by the number of days deficit/surplus.

Example:

Current range contains 2 leap days and has length 1500 days (e.g. 1 February 2024 to 11 March 2028). Next range contains 1 leap day. deltaLeapDays works out as -1. The next window length needs to be adjusted to have a length of 1499, achieved by subtracting one day from the end date. (Adjustments to the previous (left arrow) window need to be made to the newSelectedRange.start.)

There's an edge case where the current range is, for example, 2024-02-29 to 2024-02-29 (just one leap day). The new range would end up zero width, so we have to make sure the range is at least one day wide!

What do you reckon?

moontrip · 2024-07-30T13:29:18Z

@bobular Thank you for your suggestion 👍 The logic looks reasonable so I will test it. Though the function is incorrect in part as the Date is not considered correctly :)

moontrip · 2024-07-30T18:20:32Z

@bobular Ok it seems to work fine with your logic 👍 Here is a quick video capture: year 2008 is leap year

timeslider.leap.year.mp4

bobular

This is excellent. Thanks DK.

I'm not sure if the setDisableLeftArrow and setDisableRightArrow props for TimeSlider are needed? (And they are missing from useEffect dependencies in TimeSlider. They could also be combined into one callback: onBrushChange or onRangeChange.) Maybe this could be handled in TimeSliderQuickFilter in EDA instead - as part of setSelectedRange? It seems odd to refer to left and right arrows in a component that doesn't have them! Happy to have another look but you can fix and merge too if you like!

In general there seem to be some missing/extra deps that you could sort out please:

src/lib/map/analysis/TimeSliderQuickFilter.tsx
  Line 142:6:  React Hook useMemo has missing dependencies: 'extendedDisplayRange', 'studyId', and 'variableMetadata'. Either include them or remove the dependency array                                                                                                                                                                                                                react-hooks/exhaustive-deps
  Line 237:9:  The href attribute is required for an anchor to be keyboard accessible. Provide a valid, navigable address as the href value. If you cannot provide an href, but still need the element to resemble a link, use a button and change it with appropriate styles. Learn more: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/anchor-is-valid.md  jsx-a11y/anchor-is-valid
  Line 295:5:  React Hook useCallback has an unnecessary dependency: 'extendedDisplayRange'. Either exclude it or remove the dependency array                                                                                                                                                                                                                                            react-hooks/exhaustive-deps

moontrip · 2024-07-31T14:08:09Z

@bobular Thank you for your comments and suggestions. I will see if I can address those warnings: it is not always true to believe as it may sometimes cause issues (infinite loops for example).
By the way I am confused that the useEffect to deal with enabling/disabling arrows and those useState setter functions (setDisableLeftArrow and setDisableRightArrow) are all defined at TimeSliderQuickFilter where the arrow UI exists, not TimeSlider component? Did I miss or misunderstand something?

bobular · 2024-07-31T14:39:13Z

I'm not sure I follow but I think you could do something like this here

web-monorepo/packages/libs/eda/src/lib/map/analysis/TimeSliderQuickFilter.tsx

Lines 358 to 360 in b1b2c1e

    
           setSelectedRange={(selectedRange) => 
        
             updateConfig({ ...config, selectedRange }) 
        
           }

              setSelectedRange={(selectedRange) => {
                updateConfig({ ...config, selectedRange });
                setDisableLeftArrow(false);
                setDisableRightArrow(false);
              }}

bobular · 2024-07-31T14:43:54Z

Also, if required hook dependencies cause infinite loops that means the design should be revisited, in my opinion.

moontrip · 2024-07-31T14:50:50Z

@bobular Thank you for your comments. I will check them out more carefully. 👍

…web-monorepo into timeslider-date-input

moontrip · 2024-08-06T18:36:17Z

@bobular I simplified codes which were originated from old logic. It would be great if you can check it out again. Thanks!

bobular

Looks cleaner and works great still.

bobular · 2024-08-08T09:09:25Z

packages/libs/eda/src/lib/map/analysis/TimeSliderQuickFilter.tsx

@@ -234,7 +234,11 @@ export default function TimeSliderQuickFilter({
      <p>
        Expand the panel with the{' '}
        <ChevronRight transform={'matrix(0,1,-1,0,0,0)'} /> tab or click{' '}
-        <a style={{ cursor: 'pointer' }} onClick={() => setMinimized(false)}>
+        <a
+          href="/#"


I this something your IDE inserted? I think that happened once before. I don't think we want it...?

@bobular Thank you for your reviews 👍 Yes it shows warning like "no href". The "/#" actually do nothing and it is one of recommended ways: otherwise, warning message recommends to use button element, but it is not the case for this.

In Firefox the /# can show up bottom-left of the screen and I think it can end up in the address bar if clicked on. If you can remove that would be great!

Oops, I didn't check Firefox. Okay then I will remove it and merge Thanks @bobular 👍

bobular · 2024-08-08T09:49:27Z

packages/libs/components/src/components/plotControls/TimeSlider.tsx

+            : endDate,
+        });
+      }, debounceRateMs),
+    [setSelectedRange, xAxisRange, debounceRateMs, xBrushScale]


Thanks for spotting those missing deps!

bobular · 2024-08-08T09:51:25Z

packages/libs/components/src/components/plotControls/TimeSlider.tsx

+  );
+
+  // Cancel any pending onBrushChange requests when this component is unmounted
+  useEffect(() => {


test date input for time slider and adjust input element

4839c8b

moontrip requested a review from bobular May 15, 2024 12:25

applying to SAM and adjust layout

931de5b

moontrip added 2 commits May 28, 2024 22:29

add step buttons

a772b99

Merge branch 'main' into timeslider-date-input

aafb235

bobular reviewed May 31, 2024

View reviewed changes

moontrip added 2 commits June 4, 2024 16:14

address feedbacks for step buttons

2ba81b1

Merge branch 'main' into timeslider-date-input

291d302

moontrip linked an issue Jul 1, 2024 that may be closed by this pull request

Map - add forward backward step buttons to easy timeslider - add date pickers too (animation on the cheap) #775

Closed

moontrip marked this pull request as ready for review July 1, 2024 13:09

Merge branch 'main' into timeslider-date-input

f82ccf6

bobular self-requested a review July 4, 2024 10:16

Merge branch 'main' into timeslider-date-input

1716200

bobular requested changes Jul 23, 2024

View reviewed changes

address feedbacks

9efbed8

considering leap years

f8801c5

moontrip and others added 2 commits July 30, 2024 21:52

Merge branch 'main' into timeslider-date-input

b1b2c1e

updated help tooltip

d94500b

bobular approved these changes Jul 31, 2024

View reviewed changes

moontrip added 3 commits August 6, 2024 14:32

simplifying codes

16088c0

Merge branch 'timeslider-date-input' of https://github.com/VEuPathDB/…

2b32107

…web-monorepo into timeslider-date-input

Merge branch 'main' into timeslider-date-input

6bb10a9

bobular approved these changes Aug 8, 2024

View reviewed changes

moontrip added 2 commits August 8, 2024 09:24

Merge branch 'main' into timeslider-date-input

eceed45

remove href

9366590

moontrip merged commit 30425bd into main Aug 8, 2024
1 check passed

moontrip deleted the timeslider-date-input branch August 8, 2024 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test date input for time slider and adjust input element #1095

test date input for time slider and adjust input element #1095

moontrip commented May 15, 2024

moontrip commented May 21, 2024

moontrip commented May 29, 2024 •

edited

Loading

bobular left a comment •

edited

Loading

moontrip commented May 31, 2024

bobular commented May 31, 2024

moontrip commented May 31, 2024

moontrip commented Jun 4, 2024

moontrip commented Jul 1, 2024

bobular left a comment

bobular Jul 23, 2024

moontrip commented Jul 23, 2024

moontrip commented Jul 24, 2024

bobular commented Jul 30, 2024 •

edited

Loading

moontrip commented Jul 30, 2024 •

edited

Loading

moontrip commented Jul 30, 2024

bobular left a comment

moontrip commented Jul 31, 2024

bobular commented Jul 31, 2024

bobular commented Jul 31, 2024

moontrip commented Jul 31, 2024

moontrip commented Aug 6, 2024

bobular left a comment

bobular Aug 8, 2024

moontrip Aug 8, 2024

bobular Aug 8, 2024

moontrip Aug 8, 2024

bobular Aug 8, 2024

bobular Aug 8, 2024

test date input for time slider and adjust input element #1095

test date input for time slider and adjust input element #1095

Conversation

moontrip commented May 15, 2024

moontrip commented May 21, 2024

moontrip commented May 29, 2024 • edited Loading

bobular left a comment • edited Loading

Choose a reason for hiding this comment

moontrip commented May 31, 2024

bobular commented May 31, 2024

moontrip commented May 31, 2024

moontrip commented Jun 4, 2024

moontrip commented Jul 1, 2024

bobular left a comment

Choose a reason for hiding this comment

bobular Jul 23, 2024

Choose a reason for hiding this comment

moontrip commented Jul 23, 2024

moontrip commented Jul 24, 2024

bobular commented Jul 30, 2024 • edited Loading

moontrip commented Jul 30, 2024 • edited Loading

moontrip commented Jul 30, 2024

bobular left a comment

Choose a reason for hiding this comment

moontrip commented Jul 31, 2024

bobular commented Jul 31, 2024

bobular commented Jul 31, 2024

moontrip commented Jul 31, 2024

moontrip commented Aug 6, 2024

bobular left a comment

Choose a reason for hiding this comment

bobular Aug 8, 2024

Choose a reason for hiding this comment

moontrip Aug 8, 2024

Choose a reason for hiding this comment

bobular Aug 8, 2024

Choose a reason for hiding this comment

moontrip Aug 8, 2024

Choose a reason for hiding this comment

bobular Aug 8, 2024

Choose a reason for hiding this comment

bobular Aug 8, 2024

Choose a reason for hiding this comment

moontrip commented May 29, 2024 •

edited

Loading

bobular left a comment •

edited

Loading

bobular commented Jul 30, 2024 •

edited

Loading

moontrip commented Jul 30, 2024 •

edited

Loading