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

Integrate unified base mode color and helper function #1164

Merged
merged 6 commits into from
Aug 24, 2024

Conversation

louisg1337
Copy link
Contributor

Integrated e-mission-common PR which unifies the base mode colors and a helper function. Went through the code base and swapped all instances of getBaseModeByKey() with base_mode_colors.get_base_mode_by_key(). I also converted modeColors and BaseModes in diaryHelper.ts to now use our unified functions. In diaryHelper.ts I also had to add in types for one of the functions as I was getting a typescript "type unknown of distA/distB" error. I guess when I removed getBaseModeByKey(), the type inference couldn't figure out that they were numbers so it was throwing that error.

Testing Done

I went through each component that I converted getBaseModeByKey() for base_mode_colors.get_base_mode_by_key() and tested it out to make sure it still worked properly.

@JGreenlee
Copy link
Collaborator

@louisg1337

I made some changes in e-mission-common, can you update this PR?

  • bump the version of e-mission-common that is used to 0.5.4
  • fix the imports that will be broken because I renamed base_mode_colors to base_modes

@louisg1337
Copy link
Contributor Author

@JGreenlee sounds good, just made those changes in the e-mission-phone PR!

@JGreenlee JGreenlee marked this pull request as draft August 2, 2024 19:21
@JGreenlee JGreenlee marked this pull request as ready for review August 2, 2024 19:21
-remove unused imports

-fix missing type "BaseModeKey"
--this was previously `keyof typeof BASE_MODES`. Now that BASE_MODES is in e-mission-common and is defined as a `dict`, Typescript has a harder time picking up on the types. Keeping as `string` for now.
modeColors is not in diaryHelper anymore; must be accessed from e-mission-common's `base_modes`
Copy link

codecov bot commented Aug 16, 2024

Codecov Report

Attention: Patch coverage is 41.66667% with 7 lines in your changes missing coverage. Please review.

Project coverage is 30.04%. Comparing base (b390967) to head (a58e826).
Report is 78 commits behind head on master.

Files Patch % Lines
www/js/diary/details/TripSectionsDescriptives.tsx 0.00% 2 Missing ⚠️
www/js/diary/diaryHelper.ts 50.00% 2 Missing ⚠️
www/js/diary/cards/ModesIndicator.tsx 0.00% 1 Missing ⚠️
www/js/diary/timelineHelper.ts 75.00% 1 Missing ⚠️
www/js/metrics/MetricsCard.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1164      +/-   ##
==========================================
- Coverage   30.36%   30.04%   -0.33%     
==========================================
  Files         118      118              
  Lines        5256     5196      -60     
  Branches     1106     1162      +56     
==========================================
- Hits         1596     1561      -35     
+ Misses       3658     3631      -27     
- Partials        2        4       +2     
Flag Coverage Δ
unit 30.04% <41.66%> (-0.33%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
www/js/components/charting.ts 0.00% <ø> (ø)
www/js/diary/cards/ModesIndicator.tsx 0.00% <0.00%> (ø)
www/js/diary/timelineHelper.ts 92.03% <75.00%> (-0.34%) ⬇️
www/js/metrics/MetricsCard.tsx 0.00% <0.00%> (ø)
www/js/diary/details/TripSectionsDescriptives.tsx 0.00% <0.00%> (ø)
www/js/diary/diaryHelper.ts 69.76% <50.00%> (+0.53%) ⬆️

... and 6 files with indirect coverage changes

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.

Can you also include some screenshots to show that there are no regressions before I merge?

www/js/diary/details/TripSectionsDescriptives.tsx Outdated Show resolved Hide resolved
There are no usages of this
@JGreenlee
Copy link
Collaborator

<ModesIndicator> is showing the correct colors and icons of base modes


Also seen on LabelDetailsScreen with <TripSectionsDescriptives> showing colors & icons of modes on multimodal trip

@shankari shankari merged commit 02e0485 into e-mission:master Aug 24, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Tasks completed
3 participants