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

Custom Labels Step 2 #1120

Merged
merged 49 commits into from
Jul 20, 2024
Merged

Custom Labels Step 2 #1120

merged 49 commits into from
Jul 20, 2024

Conversation

jiji14
Copy link
Contributor

@jiji14 jiji14 commented Dec 29, 2023

Related Issue

What I added

  1. Manage Custom Labels on Profile Tab.
  2. Add Insert / Delete Functions

Result


Please check out to customize-modes branch in the Server repository for testing.

@jiji14 jiji14 changed the title Customize modes step2 Custom mode step2 Dec 29, 2023
@jiji14 jiji14 marked this pull request as draft December 29, 2023 21:37
@jiji14 jiji14 changed the title Custom mode step2 Customize modes step2 Dec 29, 2023
@jiji14 jiji14 changed the title Customize modes step2 Custom Labels step2 Jan 9, 2024
@jiji14 jiji14 marked this pull request as ready for review January 12, 2024 18:23
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: Patch coverage is 0% with 76 lines in your changes missing coverage. Please review.

Project coverage is 30.09%. Comparing base (b390967) to head (52b0fb9).
Report is 19 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1120      +/-   ##
==========================================
- Coverage   30.36%   30.09%   -0.27%     
==========================================
  Files         118      118              
  Lines        5256     5203      -53     
  Branches     1106     1113       +7     
==========================================
- Hits         1596     1566      -30     
+ Misses       3658     3635      -23     
  Partials        2        2              
Flag Coverage Δ
unit 30.09% <0.00%> (-0.27%) ⬇️

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

Files Coverage Δ
www/js/control/ProfileSettings.tsx 0.00% <0.00%> (ø)
www/js/App.tsx 0.00% <0.00%> (ø)
www/js/survey/multilabel/MultiLabelButtonGroup.tsx 0.00% <0.00%> (ø)
www/js/control/CustomLabelSettingRow.tsx 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

The `Icon` component was added in 5.11.0 and is referenced in CustomLabelSettingRow.
So we must use 5.11.0 at minimum
@JGreenlee
Copy link
Collaborator

@jiji14 Can you change target branch to master?

@jiji14 jiji14 changed the base branch from service_rewrite_2023 to master February 9, 2024 19:42
@jiji14
Copy link
Contributor Author

jiji14 commented Feb 9, 2024

@jiji14 Can you change target branch to master?

Done!

@JGreenlee
Copy link
Collaborator

I noticed a few issues while testing this.

I don't know if they were present before; they could be new issues from merging in changes from upstream. I also merged in Step 1 here so we can be sure Step 1 and Step 2 are working together.

Either way, @jiji14 could you take a look?

Custom Labels from label screen don't appear on profile screen

Go to the Profile screen > Manage Custom Labels and see the list of current labels (if any). Then go to the Label screen and add a custom label (via "Other..."). Then go back to the Profile screen > Manage Custom Labels, the new label does not appear immediately. After reloading, the new label does appear there

Custom labels don't appear on label screen until you interact with the "Other" input

On the label screen, I can go to "Other" and add a new custom label. Then if I go to label a new trip, the custom label shows (as expected). But if I kill the app and re-open the app, they appear to have disappeared. When I add a new custom label, I see the old ones again.

@jiji14
Copy link
Contributor Author

jiji14 commented Feb 16, 2024

@JGreenlee

The error occurred after merging. I fixed the bugs and also took care of the code coverage issue

Copy link
Collaborator

@JGreenlee JGreenlee left a comment

Choose a reason for hiding this comment

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

I tested this again and it works really well! The bugs from merging in were probably my mistake (bad merge conflict resolution).

Below are 3 quick suggestions I have, once they are applied I will pass this along!

jest.config.js Outdated Show resolved Hide resolved
www/js/control/ProfileSettings.tsx Outdated Show resolved Hide resolved
www/js/App.tsx Outdated Show resolved Hide resolved
jiji14 and others added 3 commits February 16, 2024 22:05
Co-authored-by: Jack Greenlee <JackAGreenlee@gmail.com>
Co-authored-by: Jack Greenlee <JackAGreenlee@gmail.com>
Co-authored-by: Jack Greenlee <JackAGreenlee@gmail.com>
@jiji14
Copy link
Contributor Author

jiji14 commented Feb 17, 2024

@JGreenlee
Thanks for all the suggestions. It looks great so I updated the codes :)

@shankari
Please note that server code needs to be merged first before this PR.

@shankari
Copy link
Contributor

@jiji14 or @JGreenlee can you resolve the merge conflict? I will likely target another staging release for this week so we can be testing next week.

@JGreenlee
Copy link
Collaborator

resolved

shankari added a commit to e-mission/e-mission-translate that referenced this pull request Jul 20, 2024
Support e-mission/e-mission-phone#1120 and other changes found while adding it. @Abby-Wheelis we may want to check with the Lao team and sync up changes (potentially using @JGreenlee's script) since the custom label changes will be fairly visibile.
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'm going to go ahead and merge this since it is correct, but I think it might need enhancements as part of the CO2 calculations. Right now, when the user adds a new custom mode, we don't store any CO2 information as part of it. It seems like we should, but it is not clear that the end user knows the energy/emissions intensity of the mode that they add.

I can put this up on staging/nrel-commute for now, but we should really decide how we are going to handle this before we move to production in general, since otherwise we have to deal with data migration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added Spanish translations in e-mission/e-mission-translate@e658a1a. Still need to add Lao translations @Abby-Wheelis

import { initCustomDatasetHelper } from './metrics/customMetricsHelper';
import AlertBar from './components/AlertBar';
import Main from './Main';

export const AppContext = createContext<any>({});
const CUSTOM_LABEL_KEYS_IN_DATABASE = ['mode', 'purpose'];
Copy link
Contributor

Choose a reason for hiding this comment

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

@JGreenlee don't we need this for replaced mode as well? Or is the expectation that the replaced mode will also share the custom labels from the regular mode?

Comment on lines +211 to +213
{(modalVisibleFor === 'MODE' ||
modalVisibleFor === 'REPLACED_MODE') &&
t('trip-confirm.custom-mode')}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see that we do in fact use the same values for both MODE and REPLACED_MODE

@shankari shankari merged commit 90595e9 into e-mission:master Jul 20, 2024
6 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
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants