TimelineContext fixes #1159
TimelineContext fixes #1159
Conversation
'timelineMap' is a Map object, not just a plain { } object. So instead of Object.values() we should use Map.values() - which returns an iterator - but we want to reverse it.
So we have to convert to array, then reverse, and then call forEach to fillLocationNamesOfTrip.
Also, we should ensure that only trips, not places, get passed to fillLocationNamesOfTrip
(since places always have the same locations as the trips surrounding them, the place cards will automatically get their location names filled in)
Leaflet map encounters an error when prerendered, so we need to render the TimelineScrollList component when the active tab is 'label' 'shouldUpdateTimeline' state is used to determine whether to render the TimelineScrollList or not
The existing implementation used pub/sub events to trigger when filtering should happen. The events were triggered from TimelineContext. However, this doesn't listen / wait for filterInputs to be defined because filterInputs is inside LabelTab, not TimelineContext. Filtering wasn't working because filterInputs was still empty when the initial filtering occured and LabelTab had no control over when the event was triggered. A better solution is to listen for changes inside LabelTab. Every time the filters, the loaded trips, or the labels change, filtering occurs. If a trip had a user input in the last 30s, we skip it; but schedule a re-filter when its "immunity" expires. Now LabelTab is responsible for all the filtering. TimelineContext doesn't have to worry about what LabelTab (its child) does. This is a cleaner, "React-recommended" unidirectional pattern
|
Observed in testing
|
The 'showsIf' conditions in dfc-fermata currently reference 'confirmedMode'. We want to rename this to 'primary_ble_sensed_mode'. Also see JGreenlee/e-mission-common@0396339
| useEffect(() => { | ||
| const { setShouldUpdateTimeline } = timelineContext; | ||
| // update TimelineScrollList component only when the active tab is 'label' to fix leaflet map issue | ||
| setShouldUpdateTimeline(!index); |
There was a problem hiding this comment.
I am fine with this for now, but it relies on the fact that the "label" tab is first. That seems fairly fragile - what if we reorder the tabs later? What if we start with the dashboard as the default? Isn't there a way to check the tab/route label instead?
| @@ -50,6 +50,8 @@ type ContextProps = { | |||
| loadMoreDays: (when: 'past' | 'future', nDays: number) => void; | |||
| loadSpecificWeek: (d: string) => void; | |||
| refreshTimeline: () => void; | |||
| shouldUpdateTimeline: Boolean; | |||
| setShouldUpdateTimeline: React.Dispatch<React.SetStateAction<boolean>>; | |||
There was a problem hiding this comment.
For my understanding/edification: Why is only this wrapped in React.Dispatch when (say) setDateRange is not?
| publish('applyLabelTabFilters', { | ||
| timelineMap, | ||
| timelineLabelMap: newTimelineLabelMap, | ||
| }); |
There was a problem hiding this comment.
Agreed that moving this into the LabelTab allows us to keep the presentation logic largely in the label which is the right thing to do.
|
@JGreenlee one test is failing |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1159 +/- ##
==========================================
+ Coverage 26.57% 28.92% +2.35%
==========================================
Files 114 115 +1
Lines 4993 5064 +71
Branches 1033 1094 +61
==========================================
+ Hits 1327 1465 +138
+ Misses 3666 3597 -69
- Partials 0 2 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@JGreenlee it looked like that was a timing issue (you may want to look at the prior run to see what was going on). |
|
I don't see a run where any of the Jest tests failed. All I see is codecov failing because of the token |
|
Maybe I don't fully understand what where we were supposed to be with the TimelineContext, but I thought from the code, that we would be passing in entries from the timeline to the metrics in the dashboard. However, even with the changes, I still see that recently labeled trips are not reflected in the dashboard
|
|
I don't see any obvious errors, though, so pushing out a release to staging anyway |
It was the second attempt of the run that finally succeeded |
|
Update: After restarting the app, the dashboard was updated (but active minutes appears to be blank)
|
|
And even after refreshing, the charts seem to be inconsistent with the text in the tables.
|
|
@JGreenlee and @jiji14 for visibility |
fix address names not loading
'timelineMap' is a Map object, not just a plain { } object. So instead of Object.values() we should use Map.values() - which returns an iterator - but we want to reverse it.
So we have to convert to array, then reverse, and then call forEach to fillLocationNamesOfTrip.
Also, we should ensure that only trips, not places, get passed to fillLocationNamesOfTrip
(since places always have the same locations as the trips surrounding them, the place cards will automatically get their location names filled in)
update TimelineScrollList only when the active tab is 'label'
Leaflet map encounters an error when prerendered, so we need to render the TimelineScrollList component when the active tab is 'label'
'shouldUpdateTimeline' state is used to determine whether to render the TimelineScrollList or not
fix LabelTab filtering
The existing implementation used pub/sub events to trigger when filtering should happen. The events were triggered from TimelineContext.
However, this doesn't listen / wait for filterInputs to be defined because filterInputs is inside LabelTab, not TimelineContext. Filtering wasn't working because filterInputs was still empty when the initial filtering occured and LabelTab had no control over when the event was triggered.
A better solution is to listen for changes inside LabelTab. Every time the filters, the loaded trips, or the labels change, filtering occurs. If a trip had a user input in the last 30s, we skip it; but schedule a re-filter when its "immunity" expires.
Now LabelTab is responsible for all the filtering. TimelineContext doesn't have to worry about what LabelTab (its child) does. This is a cleaner, "React-recommended" unidirectional pattern