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

Review Unified Label/Diary Screen PRs #779

Closed
sebastianbarry opened this issue Aug 22, 2022 · 8 comments
Closed

Review Unified Label/Diary Screen PRs #779

sebastianbarry opened this issue Aug 22, 2022 · 8 comments
Assignees
Labels

Comments

@sebastianbarry
Copy link

Tyler left 2 PRs open relating to the Unified label and Unified Diary screens.

Unified Label screen: e-mission/e-mission-phone#871

To-Do

  • Wait for Larbi to respond
  • Then, see if we need to pull over all the changes that it made (it made a very large amount of changes that are probably unnecessary)

Unified Diary Screen: e-mission/e-mission-phone#870

To-Do

  • implement your recommendations they seem like they are fairly simple and should not take too long.
  • restore the diary screen but with the diary item directive we can't actually remove the diary screen since we don't yet have the ability to jump to a particular date or show draft trips or show trip percentages. Once we do that, this change is essentially an improved label screen (correct me if I am wrong).
@sebastianbarry
Copy link
Author

I created 2 comments on Tyler's PR, which includes commits to do the To-Do for Unified Diary Screen:

e-mission/e-mission-phone#870 (comment)

e-mission/e-mission-phone#870 (comment)

Overall Changes:

  • I noticed in the Dashboard tab that one of the texts is still the old teal-color (see screenshot above)
  • To go into the trip details, you must click the 3 dots. I feel like it would make more sense to bring you into the trip details screen if you click anywhere on the trip tile, not just on the 3 dots
  • The color for the buttons on the Profile tab seems a bit too light colored to me personally. I feel like we'd benefit from making the red button a little darker red, and the gray buttons a little bit darker gray
  • restore the diary screen

Testing:

Screen.Recording.2022-08-23.at.10.48.52.PM.mov

Image
Image
Image
Image

@sebastianbarry sebastianbarry moved this from Current week sprint to Ready for review in OpenPATH Tasks Overview Aug 24, 2022
@shankari
Copy link
Contributor

@sebastianbarry did you confirm that all of the comments that I had left on Tyler's changes are now addressed?

@shankari
Copy link
Contributor

To incorporate your changes, just push them to your repo and create a new PR. the new PR will include all of tyler's commits, listed as authored by Tyler, and your two commits, listed as authored by you.

Next steps before I can merge this:

@shankari shankari moved this from Ready for review to Current week sprint in OpenPATH Tasks Overview Aug 28, 2022
@sebastianbarry
Copy link
Author

I created a new PR containing the changes I made to the Unified diary screen branch: [New] Unified diary screen (adapted from Tyleryandt18's Unified diary screen) e-mission/e-mission-phone#883

The old Unified diary screen is good to be closed: e-mission/e-mission-phone#870

@sebastianbarry
Copy link
Author

Make sure these "changes requested" are all completed. Go through the list: e-mission/e-mission-phone#870 (review)

@sebastianbarry
Copy link
Author

sebastianbarry commented Aug 31, 2022

Next steps

  • Remove any instance of the font-family: 'Lato'
  • In main.diary.css, change the font size back to 0.8em !important from 12px
  • In www/templates/survey/multilabel/multi_label_ui.htm change the openPopover() instances to Action sheets: https://www.tutorialspoint.com/ionic/ionic_js_action_sheet.htm
  • Remove any instance of colors which are not in the NREL Official color pallette, replace with NREL Official colors
  • Unify the detail screens

@sebastianbarry
Copy link
Author

I have gone through the code review changes in the old (Tyler's) PR, and pushed them to my new PR: e-mission/e-mission-phone#883

There are still a handfull of things to go over; any conversation from that old PR code review that doesn't end in Confirmed this change as complete still needs to be addressed.

@shankari
Copy link
Contributor

shankari commented Dec 7, 2022

This has been merged, tested and just deployed to production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants