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

📊 Rewrite of the New Dashboard Tab #1018

Merged

Conversation

JGreenlee
Copy link
Collaborator

ctyrrellnrel and others added 10 commits July 11, 2023 16:19
- replaced normal details view of 'My distance' with chart
- next steps
    - create button to switch between chart and details view in 'my distance' card
    - Figure out why all data is showing up unlabled
    - Allow the axis to switch easily
        - right now, setting isHorizontal doesn't completely switch data, only thedirection of the bars
        - would be nice to flip data axis and bar axis simultaneously
- one (MetricDetails) is going to be a copy of the original metric details view
- the next (MetricsCard) is going to be a container to have both the BarChart component, and the MetricDetails component
    - Will be a react component, rather than function, and will hold onto the state of whether or not the barchart or metricdetails will be showing

- next steps
    - add a button to switch between the two views
- allows the card to keep track of whether or not it has a BarChart or MetricDetails child component
- next step
    - Add button
- next step
    - format and style button
- button allows switching between the BarChart and the MetricDetails card
- button icon switches depending on the card represented
- removed children property from card
- removed leftover code from copying from BarChart.tsx to MetricsCard and MetricDetails
- Removed unecessary import statements
-- also switched main-metrics html so that only the MetricsCard element is held in the ion-slide element, without any extra divs or headers
@JGreenlee
Copy link
Collaborator Author

When rewriting the other screens, we have taken a bottom-up approach where we take the existing UI and gradually refactor it into components, starting from building blocks and working up to the larger pieces - and eventually we get to the main views or screens.

However, the existing Dashboard is harder to do this for because it is not organized into components at all. It exists as one very dense view+controller. The template main-metrics.html is 250 lines long and contains almost everything for the entire Dashboard tab. metrics.js is 1450 lines long and is largely opaque to me in my understanding of how it works.

I am starting to wonder if it would be more efficient to tackle this in a more 'top-down' way and not get caught up in how the old dashboard did things.
At the end of all this, we will want a Screen component that is a function of some input data, and its output is the rendered UI that we came up with in e-mission/e-mission-docs#922.

That input data definitely includes whatever we receive from the server - plus maybe a few things that are stored on the phone. (But in my experience using the Dashboard, it doesn't seem like unprocessed labels are considered for the computation of metrics, so maybe server data is the only input)

Maybe, instead of gradually refactoring into UI components under the existing Angular architecture, we should start with what data is received from the server and then figure out how to represent that in a component-based architecture.

Thoughts? @Abby-Wheelis @shankari

@JGreenlee
Copy link
Collaborator Author

JGreenlee commented Aug 21, 2023

For each of 1) user metrics and 2) aggregate metrics, I can see that we receive an array with 4 entries, each of which contain an array of objects.

image

I think the 4 entries of the outer array are the four metrics we are evaluating: 1) distance, 2) trip count, 3) duration, and 4) speed.
And I think the objects in each inner array correspond to a particular day:
image
(this is August 7, August 8, August 9)

@Abby-Wheelis
Copy link
Member

tackle this in a more 'top-down' way and not get caught up in how the old dashboard did things

I think it makes sense to start with the data we bring in from the server (and maybe the phone) and then build the pieces we want to display off of that. In a sense we will still be able to build one component at a time, but just based off of what we want to end up with, not what we started with. I've barely looked at the code that controls the Dashboard currently, but agree that we want to avoid getting bogged down in old code, in this case in particular because our end goal is more of a redesign and less of a translation (at least compared to the Profile redesign I've been doing).

@JGreenlee
Copy link
Collaborator Author

JGreenlee commented Aug 21, 2023

At a high level, the new Dashboard screen has 3 clusters of information that it needs for its 3 main sections:

  • "My Footprint" needs computed values of 'kilograms CO2'
  • "Active minutes" just needs a list of which modes are considered "active" (and potentially a mapping of what is high intensity vs low intensity), along with the above server data.
  • "My Distance / Trips / Duration / Speed" just needs the above server data (and which units of measurement should be used for display)

@shankari
Copy link
Contributor

shankari commented Aug 22, 2023

Maybe, instead of gradually refactoring into UI components under the existing Angular architecture, we should start with what data is received from the server and then figure out how to represent that in a component-based architecture.

I am fine with this. I edited the metrics screen most recently (to support the use of user labels in the dashboard calculations) and it was an unholy mess. I refactored it to some extent in the time that I had, and it is better than before (e.g. 6e88d8e !!) but I remember that I wanted to just throw it away and rewrite but not having the time or the UX design skills to do so to do so.

Where do these currently come from? Do these computations require any input other than the above server data? Is this going to change anyway with e-mission/e-mission-docs#954?

For user labels, these come from the trip_confirm options! the "co2PerMeter" field.
For the sensed modes (if we are not returning user labels, these come from the CarbonFootprint settings)
Both of these should be handled in FootprintHelper

I don't think that the dashboard rewrite should wait for e-mission/e-mission-docs#954
That requires an integration with the EPA grid and we need to decide whether we should cache data or query every time (is querying for ~ 10 years ago still possible)? But while writing the mapping code, it would be good to keep open the ability to change the mappings. Putting them in trip_confirm_options is already better than hardcoding them in the app code.

"Active minutes" just needs a list of which modes are considered "active" (and potentially a mapping of what is high intensity vs low intensity), along with the above server data.

These are also available from the same locations - met in the trip_confirm options, for example
https://en.wikipedia.org/wiki/Metabolic_equivalent_of_task

The new dashboard will have the MetricsTab component as its entry point - main-metrics.html will no longer be rendered
Replacing main-metrics, this component represents the entire Dashboard tab and will get metrics data from the server, store it as state, and pass it down to child components like MetricsCard.

There is also a date picker at the top (TODO) and a refresh button.

The logic for fetching metrics has been modernized into modern JS (async/await) and uses Luxon instead of Moment. I tested to make sure the timestamps of the resulting queries are the same using Luxon as they were with Moment.
This typing is based on the data we receive from the server.
For each metric, we receive an array containing "days" of metric data. Each day has a few props (ts, fmt_time, nUsers, local_dt) plus any number of properties with the `label_` prefix that represent the measurment for a particular label.

Having this be type-safe from the start should make it much easier to realized the desired visualizations!
MetricsCard will now accept 'metricDataDays' as a prop and derive the chart data from this.
We can employ `useMemo` for this so that iff metricDataDays changes, chartData updates.

Prior to this, MetricsCard had an IconButton on the right side allowing users toggle between 'details' and 'graph' view.
To be clearer to the user, I have changed this to a SegmentedButtons. This way, users see both their options side by side and can select the desired one.

A few colors were added in appTheme.ts - surfaceDisabled and onSurfaceDisabled.
These are colors that already exist in the React Native Paper default theme, but we are overriding for our own purposes.
In the SegmentedButtons on MetricsCard, the surfaceDisabled color is used for the button that is not checked
With the new MetricsTab, MetricsDetails will receive 'metricDataDays', sum up the totals for each label, and display these in a 2-column.
The display is the same as the old dashboard, but now the logic to sum up the values is here in this component instead of jumbled in with everything else.
@JGreenlee
Copy link
Collaborator Author

Some progress on implementing the new design

Units of measurement are not handled yet

MetricsDateSelect will allow users to select a date range. It appears as a NavBarButton, like the label screen date picker, and also uses a DatePickerModal.
However, this one works differently because users can select a range of days instead of just one.

The selected range is stored in LabelTab as `dateRange` - once this range is loaded, all the displayed metrics will update to reflect the new data.
`dateRange` is an array of two Luxon DateTime objects, which works well to bridge between the JS Date objects (used by RN Paper Dates) and Unix timestamps (used by e-mission-server / for any network calls)
The chart does not need so much padding - we want to be able to show as much detail as we can.
MetricDetails should show with 2 columns, but could use some padding in between.

So let's remove all the padding from the card content since its children will have padding anyway.
The "active minutes" should display the cumulative duration for modes that are considered "active" - currenty this is just 'walk' and 'bike'; I am not sure if anything else will count.
This should include a graph later on, but right now it just lists out the minutes for each mode.
The same type will be used for both user metrics and aggregate metrics, so its naming should be more generic.
This adds type definitions for the React hooks and makes some of the annoying typing errors go away.
I don't know why the React package doesn't have types built-in, but this is how you get them.
It's not really necessary for MetricDetails to be its own component - it is actually simpler to just include these few lines of code in MetricsCard.
Then MetricsCard will handle the memoized computation either way, whether it's `chartData` or `metricSumValues` that is needed (depending on whether 'graph' or 'details' is active)
It was incorporated into MetricsCard and is no longer a standalone component
JGreenlee and others added 8 commits September 15, 2023 15:44
if we had dates for the 1st through 15th (15 days), this function would mistakenly return the 8th through 14th and the 1st through 7th.

We actually want the 9th through 15th and the 2nd throgh 8th, because it should be relative to the most recent day.

To fix this we iterate over the days backwards, not forwards. With this approach we also don't have to reverse the weeks when we return them because they are already with the most recent first.
we had figured out that there were some differences e-mission/e-mission-docs#961 (comment)

Eventually, we realized this was because the new dashboard was not using the custom labels.

This commit adds the methods that check to see if the labels are custom or sensed to `metricsHelper`, checks for custom labels and indicates the need for custom footprint mappings in `CarbonFootprintCard` and the finally reverts back to "rich modes" rather than "base modes" in `metrics-factory` so we use the custom labels
this hardcoded value caused the returned color to be the same for both the bar background and the border. The darkening should be based on the parameter allowing the border to use a darkened green while the background is not darkened.
This gives the bar charts a slightly softer feel, more in line with the rest of the UI which is pretty rounded off everywhere.
If the scale of the graph is too large, the 2030 and 2050 guidelines labels might overlap. In that case, we should show the 2030 label on top.

Also, the colors are kind of hard to see with the bars having their new gradient color scheme. So for the colors of the lines, let's just darken the yellow, and bump up the saturation of both.
found a bug where the text for the group was being calculated off of user metrics!
to avoid showing metrics for partial weeks, check the length before displaying.
@JGreenlee JGreenlee marked this pull request as ready for review September 18, 2023 15:10
Abby Wheelis added 2 commits September 18, 2023 13:19
we've moved away from showing "calories burned in active travel" and started to show "minutes spent in active travel" Therefore, we no longer need the keys about calories, calibrating to user date (already left profile) or comparing the calories to food.
@shankari shankari changed the base branch from label_dashboard_profile_sept_2023 to onboarding_services_sept_2023 September 24, 2023 14:03
@shankari shankari changed the base branch from onboarding_services_sept_2023 to onboarding_routing_sept_2023 September 24, 2023 14:12
@shankari
Copy link
Contributor

@JGreenlee can you resolve the merge conflict? I took a look and the difference is basically in the colors used. I assume that the colors in the dashboard-rewrite branch are correct, but would appreciate your taking a look.

Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

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

I am fine with merging this once the merge conflict has been resolved, and tackling the other changes in a fairly quick cleanup. It would also be good to pause for a bit and add tests for the metricsHelper while cleaning up.

www/index.js Show resolved Hide resolved
@@ -109,7 +109,6 @@
"angular": "1.6.7",
"angular-animate": "1.6.7",
"angular-local-storage": "^0.7.1",
"angular-nvd3": "^1.0.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

woo! Will we be restoring the graph in the trip detail screen or adding new ones (using the new chart library) going forward?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The speed graph should be fairly easy to add because we made LineChart in such a way that it can be easily re-used.
If we want, we could display plenty of other interesting things there too.

www/js/config/useImperialConfig.ts Show resolved Hide resolved
www/js/config/useImperialConfig.ts Show resolved Hide resolved
Comment on lines +4 to +5
import { getBaseModeByValue } from './diary/diaryHelper'
import { labelOptions } from './survey/multilabel/confirmHelper';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these here? None of the other changes seem related to these.

Copy link
Collaborator Author

@JGreenlee JGreenlee Sep 25, 2023

Choose a reason for hiding this comment

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

No we don't need. Leftover from a previous commit and those functions aren't used here anymore
TODO cleanup

Comment on lines +60 to +62
//modes considered on foot for carbon calculation, expandable as needed
const ON_FOOT_MODES = ['WALKING', 'RUNNING', 'ON_FOOT'] as const;

Copy link
Contributor

Choose a reason for hiding this comment

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

as long as we use outputs beyond CLEAN_AND_RESAMPLE in the pipeline, we should not need this mapping. We should clarify that we only support cleaned/inferred/confirmed values in the dashboard so that we can simplify this logic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because after cleaning, all of these would turn into 'WALKING', right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup. The RUNNING and ON_FOOT are really for data that comes in directly from the phone.

}
}
//this section handles user lables, assuming 'label_' prefix
if(field.startsWith('label_')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should be able to split the parsing of inferred versus user label modes into two separate functions instead of this giant function - e.g. the two if statements

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO cleanup

Comment on lines +172 to +178
/*
* We use the results to determine whether these results are from custom
* labels or from the automatically sensed labels. Automatically sensedV
* labels are in all caps, custom labels are prefixed by label, but have had
* the label_prefix stripped out before this. Results should have either all
* sensed labels or all custom labels.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

for the record: this should eventually go into the dynamic config - whether people want to use sensed values or user values. Or it may be moot once "count every trip" is implemented.

function getLastTwoWeeksDtRange() {
const now = DateTime.now().startOf('day');
const start = now.minus({ days: 15 });
const end = now.minus({ days: 1 });
Copy link
Contributor

Choose a reason for hiding this comment

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

for the record, we might want to revisit this later if we ever change our labeling flow to be more real-time, so that the labels are reflected in the metrics. Note also that this is in UTC, which may cause some confusion among users that we could consider addressing in a future cleanup.

import { DateTime } from "luxon";

type Props = {
dateRange: DateTime[],
Copy link
Contributor

Choose a reason for hiding this comment

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

we should set this to the first date of the study/program or the first date for the current user, similar to the "Download JSON dump" code in the profile.

@JGreenlee
Copy link
Collaborator Author

Confirmed that the merge conflict was just color values and that dashboard-rewrite had the most up-to-date palette.
I fixed the conflict and will address the rest of the review tomorrow.

@shankari
Copy link
Contributor

shankari commented Sep 25, 2023

There is now a new conflict in main.js, likely from my merge of #1029
Please move back to "ready for review" once addressed.

Abby Wheelis added 5 commits September 25, 2023 08:55
also tested this code by running in emulator to ensure no errors
since we have room in the code, clarify what we mean by referencing "how many days ago" the calculations cover
if the change is infinite, show that. If the change is infinite, and infinitely uncertain (-inf/+inf) don't show, and otherwise show the finite range
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Address code reviews with "future"
Development

Successfully merging this pull request may close these issues.

4 participants