Ez time filter component and story #320
Conversation
|
Update 1: made a time filter at SAM 8440f05 . A demo video is attached in the following. Though, there are several things to do for working, including unknowns. a) Currently, a temporary test data (a lineplot data) is used to show a kind of density plot to represent the existence of data in the date range. Not quite sure what and how data should be used for this purpose. SAM.time.filter01.mp4@bobular and @d-callan, can you please let me know how to handle above a) and b) ? |
|
a) there is this backend ticket, which more or less aims to establish a new endpoint dedicated to providing the data needed for this. the ticket describes providing a response indicating only the presence or absence of data, rather than how much data how much more work do we think it is to make the bar from the mockups? |
Thank you for your detailed information, @d-callan 👍 Honestly, it is not clear how to wire data-related one to the time filter component until I see the response from the backend, so I will wait for the backend to be ready. As for the submit button, perhaps a kind of debounce may work without submit button as you mentioned. This may not be that clear to a user whether data request is made or not, so perhaps a loading icon or something like that needs to be displayed not to change the range. Will think about it. In addition, you also mentioned about selected range as an input, which I guess you meant a kind of input form to change values. As far as I know, the sliding bar, which is made of Visx's brush component, is uncontrolled component. Thus it is very hard to manage it from parent component like map. Not quite sure if the following would work without testing, but this may be feasible by moving the range part (and reset icon) into the time filter component (EzTimeFilter.tsx). For your last comment, "make the bar", I guess you mean thin and thick horizontal bar style at mockup instead of current density plot. I thought that relevant data/response from the backend would include the format of x=time & y=value format, so in this case, it was arguably the easiest way to use Visx's xychart package and AreaClosed (like density plot) component so that x-axis tick & its label and y-axis values are shown with minimum efforts. From my understanding, there is no specific Visx component to display thin and thick horizontal bar style. This means that if we want to have such a bar style, then: 1) they should be made via SVGs manually; 2) this will also mean that x-axis should also be made via SVGs manually. That is, this would involve significant works to be implemented: basically this means that we need to make a specific plot component via d3 only. |
|
On the subject of the bars, what if we just used histo data with all bars having height 1 if there is data and 0 if no data? That could mess with the axis a bit, but maybe worth a shot @moontrip ? The other idea i came up with was doing basically ticks at every point where we have data. So we'd use Anyways, curious if either of the above might be an easier way to get closer to the mockups! |
|
thanks @asizemore. i do have a fairly strong preference to show presence/ absence of data as opposed to 'how much' data. if we can find a way to achieve that without a huge cost, that would be ideal. |
|
Thank you for sharing your thoughts @asizemore 👍 Yes your suggestions can be relatively easier approach than mockup one. Looks like bar approach is more similar to the mockup. Will test it. |
|
Hi @d-callan & @asizemore I have tested scaled values that represent with (1) or without data (0). In Visx, in general scaleBand is used for Bar plot's x-axis scaling instead of scaleTime (for, e.g., lineplot/time series, etc.). The scaleBand automatically compute bandWidth based on data and plot size, however, it cannot be used for our purpose directly because slider filter does not recognize the x-axis values which is time-based, not bin/categorical. But I found that scaleTime can also be used for Bar plot by computing bandWidth manually, which resolves the aforementioned issue. The first screenshot is a demo version. I have also tested lineplot (xychart) case with 0 and 1 values (2nd screenshot), but I guess you prefer the bar instead of having slopes between data and no data in the lineplot. As a side note, I tested debouncing functionality to possibly be used for submitting selected range, and I could implement it with slider action.
|
|
Thanks 👍 think you can make it shorter now? Possibly add a custom line annotation across the center of those bars? |
|
Thank you for your comments @d-callan 👍 Can you please confirm my guess in the following? a) so making it shorter means the height of bars (i.e., plot height)? like 2/3 of current height? b) simply adding a horizontal center line in the bar plot? |
|
I think the line plot (second screenshot above) looks great. |
|
my issue w the line is primarily that it leaves it a bit ambiguous whether there is or isnt data for July 2009 |
|
@d-callan, @asizemore, (and @bobular) I have addressed feedbacks, center line in the horizontal direction and shorter height. In addition, I figured out how to make the time filter plot be closer to the original mockup, #245. This is applied to both components' storybook and SAM, which a screenshot taken from SAM is attached FYI.
|
|
Looks very nice DK! |
Thanks @bobular 😃 |
|
beautiful! i wonder now if its possible to have the label above serve also as an input? |
Glad you like it @d-callan :) As for the label/input style, although I will need to investigate more, at this stage there are a couple of issues that need to be addressed. The biggest issue is that the slider, which is made of Visx brush, is uncontrolled component within my knowledge. Thus, there seems to be no explicit way to change the slider position by parent component (e.g., via props): in other words, even if an input range is made and the range values are changed, the slider position won't be changed, which is not good for UI/UX perspective. |
There was a problem hiding this comment.
I have a suggestion how to make the Brush behave like it is controlled. It seems to work for me (I can control it from dev tools), but would like @dmfalke's view on this too!
First, set the initial position from EzTimeFilter's selectedRange prop
const initialBrushPosition = useMemo(
() => ({
start: { x: xBrushScale(new Date(selectedRange.start)) },
end: { x: xBrushScale(new Date(selectedRange.end)) },
}),
[selectedRange, xBrushScale]
);
Then add a key to the Brush component
<Brush
key={selectedRange.start + "/" + selectedRange.end}
If this (or another method) works, then it should be relatively straightforward to add the "window forward" and "window backwards" buttons (as well as the date-picker inputs from the "title").
| paddingRight: '1.5em', | ||
| }} | ||
| > | ||
| <a |
There was a problem hiding this comment.
Is there a reason for putting the reset button outside the component?
To me it just adds complication though it has helped me understand forwardRef 🙂
Also, can we use a real button, not an <a> tag? In the storybook at least, clicking on the reset link makes everything go into fullscreen storybook mode.
There was a problem hiding this comment.
So, my (initial) intention to place the reset at parent level was for positioning. But I think that it may be better to place the reset and selected range label into the ez time filter component, and then adjust InputVariable component, which should be at parent component, via CSS positioning.
Yes, I also noticed that tag causes it at storybook. Will handle it, either change to button or something else.
|
ChatGPT3.5 seemed to reluctantly agree that the |
|
@bobular Thank you for your examination 👍 I saw something similar approach somewhere: IIRC, we also used similar one at our Plotly (for thumbnail I suppose). Will check. |
|
Hi @moontrip - did you run into problems making (faking) the controlled behaviour of the Or have you not been able to get back to this due to time constraints? |
|
Maybe @dmfalke could comment on my suggested use of the |
Hi @bobular 👋 I think that it may work but as you said, I didn't have time to test it due to time constraints. Actually I did use key approach for other purpose, attach/detach functionality of the filter to the top area and it seems to work fine: didn't commit the latest works yet as it needs some refactoring as well as I am dealing with other ticket (truncation one you commented alot 😄). |
This will work, but it could be expensive. Changing the value of the function EZFilter(props: Props) {
// Track how many times the input has changed by user
const [inputChangeCounter, setInputChangeCounter] = useState(0);
// Generate brush key based on inputChangeCounter
const brushKey = `iteration-${inputChangeCounter}`;
// ...
return (
<>
<DateInputs onChange={range => {
// Update range value in analysis state
updateRange(range);
// Increment counter
setInputChangeCounter(c => c + 1);
}}/>
<Brush key={brushKey}/>
</>
} |
|
Made lots of changes for ez time slider concerning UI and UX feedbacks. Summary is: changes
todo
A captured video is attached in the following: |
… rid of a useEffect
There was a problem hiding this comment.
I'd like to see the additions in the file moved to a file specific to the ez time slider
There was a problem hiding this comment.
I'm wondering if the business logic in the component should be factored out into a new file, packages/libs/eda/src/lib/core/components/filter/EZTimeFilter.tsx. I would remove the DraggablePanel part of it.
There was a problem hiding this comment.
I've already renamed it EZTimeFilter and removed the draggable. I haven't done any further refactoring as yet.
| timeSliderVariable: VariableDescriptor, | ||
| timeSliderSelectedRange: t.type({ | ||
| start: t.string, | ||
| end: t.string, | ||
| }), | ||
| timeSliderActive: t.boolean, |
There was a problem hiding this comment.
I'd like to see this all combined into one object, something like:
timeSliderConfig: {
variable: VariableDescriptor,
selectedRange: t.type({
start: t.string,
end: t.string,
}),
isActive: t.boolean,
}
| setTimeSliderVariable: useSetter('timeSliderVariable'), | ||
| setTimeSliderSelectedRange: useSetter('timeSliderSelectedRange'), | ||
| setTimeSliderActive: useSetter('timeSliderActive'), |
There was a problem hiding this comment.
Similar to my other comment, this could be
setTimeSliderConfig: useSetter('timeSliderconfig')
There was a problem hiding this comment.
I just made an observation: the date filters that get generated by the ez time filter are missing the time component, which causes a backend error, so that needs to be addressed somehow.
A related thought I just had is, perhaps appState can store a filter object? E.g.,
ezTimeFilterConfig: {
isActive: boolean,
filter?: Filter
}
| // take the first suitable variable from the filtered variable tree | ||
|
|
||
| // first find the first entity with some variables that passed the filter | ||
| const defaultTimeSliderEntity: StudyEntity | undefined = Array.from( | ||
| preorder(temporalVariableTree, (entity) => entity.children ?? []) | ||
| ) | ||
| // not all `variables` are actually variables, so we filter to be sure | ||
| .filter( | ||
| (entity) => | ||
| entity.variables.filter((variable) => Variable.is(variable)) | ||
| .length > 0 | ||
| )[0]; | ||
|
|
||
| // then take the first variable from it | ||
| const defaultTimeSliderVariable: Variable | undefined = | ||
| defaultTimeSliderEntity.variables.filter( | ||
| (variable): variable is Variable => Variable.is(variable) | ||
| )[0]; |
There was a problem hiding this comment.
I ran into a runtime bug with this code, when I was playing with constraints. For some reason, typescript is allowing you to assume that defaultTimeSliderEntity is defined on line 316, even thought it might be null. The suggested change below will address this, and I think it's also a little more performant (filter will iterate the entire array, while find and some will terminate after the first element for which the callback returns true).
| // take the first suitable variable from the filtered variable tree | |
| // first find the first entity with some variables that passed the filter | |
| const defaultTimeSliderEntity: StudyEntity | undefined = Array.from( | |
| preorder(temporalVariableTree, (entity) => entity.children ?? []) | |
| ) | |
| // not all `variables` are actually variables, so we filter to be sure | |
| .filter( | |
| (entity) => | |
| entity.variables.filter((variable) => Variable.is(variable)) | |
| .length > 0 | |
| )[0]; | |
| // then take the first variable from it | |
| const defaultTimeSliderVariable: Variable | undefined = | |
| defaultTimeSliderEntity.variables.filter( | |
| (variable): variable is Variable => Variable.is(variable) | |
| )[0]; | |
| // take the first suitable variable from the filtered variable tree | |
| // first find the first entity with some variables that passed the filter | |
| const defaultTimeSliderEntity = Array.from( | |
| preorder(temporalVariableTree, (node) => node.children ?? []) | |
| ).find((entity) => entity.variables.some(Variable.is)); | |
| // then take the first variable from it | |
| const defaultTimeSliderVariable = defaultTimeSliderEntity?.variables.find( | |
| Variable.is | |
| ); | |
| return defaultTimeSliderEntity != null && | |
| defaultTimeSliderVariable != null | |
| ? { | |
| entityId: defaultTimeSliderEntity.id, | |
| variableId: defaultTimeSliderVariable.id, | |
| } | |
| : undefined; |
There was a problem hiding this comment.
Furthermore, when this function returns undefined, the MapAnalysisImpl component enters and infinite loop.
|
Hi @dmfalke - thanks again for your pre-review. I've now refactored the It's working the same as before, even though I haven't wrapped the |
Resolves #245.
It is now wired up to the user analysis saved state and the slider is working smoothly except for a known Firefox bug (see #480). The ez time filter now applies to markers and floaters. But not downloads. I added an on/off toggle, feedback welcome on this.
Remaining subtasks - in no particular order, and many could be done in parallel
timeSliderActiveisfalse(e.g. toggled off)- create a super slim minimised mode
- expand/collapse button (or click on the minimised mode to expand)
- user interactions should only occur in the maximised mode, but the following basic information should be communicated in minimised mode:
1. the variable name (plain text, ellipses as required)
2. the distribution (no x-axis tick labels) and blue selected region if active (grey if inactive?)
DK's original notes mainly about the slider component (click to expand)
It is primarily based on Visx Brush component, thus I added @visx/brush at web-components's package.json/dependencies. Also made a story file to test.
Some notes are: a) the plot area is made of a kind of density plot for now. It is because it looks good and the mockup one will probably need much more work; b) an example data was taken from Lineplot data (GEMS1 Case Control; x: Enrollment date; y: Weight) for demo purpose; c) the selected range (or filtered area) is expressed as diagonal lines so that background plot (density plot) can be seen; d) initial position of the filter/selection is set to be whole range; e) the brush handle, which is for mouse drag to slide, is currently shown as small rectangles. It is a typical shape that can be found in other tools so I leave it as it: it seems like one can customize it though.
Here is a demo video:
EzTimeFilter01.mp4