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

Organize client folder & refactor NavBar & Fix i18 text #720

Merged

Conversation

AdhamAH
Copy link
Member

@AdhamAH AdhamAH commented May 16, 2020

⚠️ IMPORTANT: Please do not create a Pull Request without creating an Issue first.

All changes need to be discussed before proceeding. Failure to do so may result in the pull request being rejected.

Before submitting a pull request, please be sure to review:


Please include the issue number the pull request fixes by replacing YOUR-ISSUE-HERE in the text below.

Fixes #709 fixes #719 fixes #715

Summary

  1. refactor NavBar.jsx
  2. reorganize client folder

Details

Test Plan (required)

Screenshot 2020-05-17 at 01 07 42

Screenshot 2020-05-17 at 00 22 04
Screenshot 2020-05-17 at 00 23 29
Screenshot 2020-05-17 at 01 05 57
Screenshot 2020-05-17 at 01 06 41

Final Checklist

  • For CoronaTracker, did you bump the version in package.json?
    • Update the second decimal for a major change
    • Update the third decimal for a minor change
    • Numbers can go past 9, e.g. 1.0.9 => 1.0.10
    • For more info, read about Semantic Versioning
  • Did you add any new tests as necessary?
  • Is your PR rebased off the most current master?
  • Have you squashed all commits? (can be done at merge)
  • Did you use yarn not npm? (important!)
  • Did you use Material-UI wherever possible?
  • Did you run yarn lint on the code?
  • Did you run all of your most recent changes locally to make sure everything is working?

@AdhamAH AdhamAH changed the title Organize client folder Organize client folder & refactor NavBar & Fix i18 text May 16, 2020
SomeMoosery
SomeMoosery previously approved these changes May 17, 2020
Copy link
Member

@SomeMoosery SomeMoosery left a comment

Choose a reason for hiding this comment

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

Looks good to me at first glance, and this is definitely a much-needed adjustment. Nice work!

I'm not too sure what the refactor nav bar issue is, and if it's maybe a duplicate of the issue I made #716 at what appears to be at the same time? :)

@AdhamAH
Copy link
Member Author

AdhamAH commented May 17, 2020

Thank you! Yeah I saw the issue you made after I opened #719 😄
It is a close issue but it is different. What I did here not made the changes in #716 but I changed what component the NavBar uses to something more suitable to make the changes to UI and NavBar easier to apply.

ngiangre
ngiangre previously approved these changes May 20, 2020
Copy link
Contributor

@ngiangre ngiangre left a comment

Choose a reason for hiding this comment

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

LGTM!

crispinamuriel
crispinamuriel previously approved these changes May 21, 2020
@SomeMoosery
Copy link
Member

While this all looks good and it should be ready to merge, I'd like to get one more codeowner approval on this before merging in (@acthelemann @pavel-ilin)

Otherwise, i'll just merge it tomorrow though as we should really get this in

pavel-ilin
pavel-ilin previously approved these changes May 21, 2020
@SomeMoosery SomeMoosery dismissed stale reviews from pavel-ilin, crispinamuriel, ngiangre, and themself via 56d9300 May 22, 2020 03:01
SomeMoosery
SomeMoosery previously approved these changes May 22, 2020
Copy link
Member

@SomeMoosery SomeMoosery left a comment

Choose a reason for hiding this comment

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

Apologies, wasn't thinking and dismissed stale reviews when I had to update after merging in other PRs. Needs a quick approval then should be good to go.

* update calendar month hover font

* updated calendar tile color

* update calendar with new hover color

Co-authored-by: Carter Klein <carterklein13@gmail.com>
@AdhamAH
Copy link
Member Author

AdhamAH commented May 22, 2020

No worries at all @SomeMoosery
I actually could merge it before I went to sleep but I didn't. I was waiting for the two other PR to merge. It is easier for me to rebase, hopefully 😄

Copy link
Contributor

@ngiangre ngiangre left a comment

Choose a reason for hiding this comment

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

LGTM!

@AdhamAH AdhamAH merged commit 2d2960c into COVID-19-electronic-health-system:master May 22, 2020
@AdhamAH AdhamAH deleted the organize-client-folder branch May 22, 2020 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants