COVID-19-electronic-health-system / Corona-tracker

An easy-to-use PWA to monitor the user's wellness and learn about COVID-19.
https://coronatracker.me/
MIT License
234 stars 101 forks source link

update calendar month hover font to black #711

Closed johnemurphy1 closed 4 years ago

johnemurphy1 commented 4 years ago

⚠️ 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 #542

Summary:

This fixes the white font against yellow background on month in calendar when user hovers over month by making font black.

Details

See summary above.

Test Plan (required)

Minor CSS change.

Final Checklist

johnemurphy1 commented 4 years ago

@AdhamAH I realize I didn't change the font color for all the different components now. Is that what you are saying doesn't answer issue #542? Please let me know.

AdhamAH commented 4 years ago

Do you see the yellow colour? I think this is what the issue about

johnemurphy1 commented 4 years ago

yes, i see the yellow color. the proposed solutions were (on the hover) 1. change the font color to black or 2. change the background to dark blue. I propose just changing the font to black.

SomeMoosery commented 4 years ago

I personally don't think I read the issue carefully enough, but I'm not sure why we'd want that background to be yellow. @whoabuddy was there any reason you'd propose changing the text as opposed to the background color? I'm not sure if the yellow is capturing any information here 🤔

acthelemann commented 4 years ago

Did you add this translationsFormatted.json file? If not you need to rebase off master.

Even if you did, we shouldn't add that file in this commit.

Also, I don't think your change makes the font color black when necessary. Nothing seems to have changed when I run it locally. Can you post a screenshot/gif of it working on your end?

johnemurphy1 commented 4 years ago

@SomeMoosery and @AdhamAH what is the consensus on how this issue should be handled? Change the background color to the darker blue? Could you please let me know? Then, I'll work on that.

@SomeMoosery and @acthelemann . The translationsFormatted.json file was something I worked on with @awu556 for @salvolpe. I believe @ngiangre and @BrianHHough (?) took it over and that file needs to be sunsetted. It was started a while back and left on the shelf. Should not have been in this PR. My mistake.

ngiangre commented 4 years ago

@johnemurphy1 yes we've taken that and used it to refactor into the translation.json files in client/locales/

acthelemann commented 4 years ago

@johnemurphy1 I think you should mimic the styling on this hasActive class, so you'll be changing the default background color and the background color on hover/focus.

@AdhamAH @SomeMoosery @whoabuddy Thoughts?

The styling on the calendar is messy in general and should be consolidated all in Calendar.jsx. Calendar.css should be removed. That can be done in another PR/issue, though.

AdhamAH commented 4 years ago

The styling on the calendar is messy in general and should be consolidated all in Calendar.jsx. Calendar.css should be removed. That can be done in another PR/issue, though.

@acthelemann I totally agree and I actually took a look before how could it be done but I was busy to do it. But this is out of #542 scoop.

@johnemurphy1 what needs to be done with this issue is changing the background of the current date of the day/month/year (yellow) when selecting a different date and hover over the current date.

whoabuddy commented 4 years ago

I personally don't think I read the issue carefully enough, but I'm not sure why we'd want that background to be yellow. @whoabuddy was there any reason you'd propose changing the text as opposed to the background color? I'm not sure if the yellow is capturing any information here

Just for simplicity - black text would make it readable, but eliminating the yellow is better!

johnemurphy1 commented 4 years ago

Updated the hover with new dark blue color to make white font legible.