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
235 stars 101 forks source link

restructuring show me more #638

Closed AdhamAH closed 4 years ago

AdhamAH commented 4 years ago

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

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

Please be sure to review our Code of Conduct, Contributing Guidelines, and Support documentation before submitting a pull request.


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

Fixes #591 and Fixes #590

Summary

restructuring show me more

Details

1- Followed the advice of @acthelemann to use ExpansionPanel for the observations 2- deleted unused files 3- updated some styles around the app to fit changes 4- removed bootstrap

Test Plan (required)

show

Final Checklist

AdhamAH commented 4 years ago

Will do the changes @SomeMoosery first I needed to make sure the rebase worked and it did

acthelemann commented 4 years ago

@AdhamAH I can't run this locally because observations.jsx is lowercase

AdhamAH commented 4 years ago

Screenshot 2020-04-24 at 22 01 38 I changed it before pushing! I am not sure why it didn't change here

acthelemann commented 4 years ago

I'm not 100% sure if these changes broke the WeeklyTrackerDay component swiping

SomeMoosery commented 4 years ago

Yeah, I'm having issues too locally, without checking out this branch.

Last person to edit implementation of WeeklyTrackerDay is @Bellex0 here - but that seems like too long ago to have gone unnoticed. This should probably made into a bug report though, which I can make

AdhamAH commented 4 years ago

I just tried it too, it was there before these changes, so I will rebase again and we will open a separate issue for that? @SomeMoosery

acthelemann commented 4 years ago

Yeah, that should be a separate issue, but the question that that issue, and this PR, is dependent on remains: Do we want it to be swipeable? If yes, what do we want it to do? Go to "Show Me More" with just that observation showing?

SomeMoosery commented 4 years ago

I'm personally on board with removing the swipe functionality and making it clickable, as people have been complaining about swiping on a web app (since not a native app). Would that fall under this issue though or should we open a new one?

acthelemann commented 4 years ago

I'm personally on board with removing the swipe functionality and making it clickable, as people have been complaining about swiping on a web app (since not a native app). Would that fall under this issue though or should we open a new one?

That would also be a separate issue, but I'm asking if we want the click/swipe handling in the first place. If we do, then we'll need that detailData.length ? detailData : observations line from before that only displays the one observation in the "Show Me More" screen

AdhamAH commented 4 years ago

That was my intention to keep things as before So do you want me to put it back? @acthelemann @SomeMoosery

SomeMoosery commented 4 years ago

Oh, okay, I see - sorry for causing confusion. Yes, let's put that back. I wasn't even suggesting we remove it originally, was just asking what purpose it served