Split CSS file - Githubissues

HarshKapadia2 / one-or-two

A hand gesture model web app to detect 1 and 2 in a picture.

https://one-or-two.herokuapp.com/

MIT License

5 stars 4 forks source link

Split CSS file #15

Closed Haroonchik closed 4 years ago

Haroonchik commented 4 years ago

I split the CSS file, tweaked some styling and fixed one bug in app.py that caused the application to crash

HarshKapadia2 commented 4 years ago

Thank you for the PR @Haroonchik! Sorry for the delay in the review, I have mid term exams going on right now.

HarshKapadia2 commented 4 years ago

I split the CSS file, tweaked some styling and fixed one bug in app.py that caused the application to crash

You have made too many changes in one PR. This PR should be three separate PRs, namely:

Split the CSS file
Tweaked styling
Fixed bug in app.py

Please remember this next time. It becomes difficult for the maintainer/owner to suggest and find changes. The smaller the PR (in terms of the number of issues handled in each), the better.

Also, your commit message (Split CSS file) does not reflect the other changes that you have made. There should have been three commits for the three major changes.

HarshKapadia2 commented 4 years ago

Okay so getting to this PR. The code splitting (issue #9) is fine.

In the additional changes, there are some things to be handled:

I don't agree with the variable names finger and peace as they can be confusing for someone. The previous variable names output_1 and output_2 were clearer.
The styling changes aren't responsive in mobile view (I used 360x640 as the dimensions to check.) so please correct that for all pages.
The file input and 'Predict' buttons are too big on the home page. Just reduce their size a little. You can tweak the CSS in their respective files and as the page specific files are loaded after the base.css file, the code in base.css will be overridden for those particular properties.

Haroonchik commented 4 years ago

Took your comments, thanks. Now I'm going to start adjusting the styles. If I adapt the page for a smartphone and a computer, will that suit you?

Haroonchik commented 4 years ago

@HarshKapadia2 Check, please

HarshKapadia2 commented 4 years ago

Thank you for the changes! The two boxes look quite squished. Could you make it a little bigger? Maybe increasing the font size of the icon and text will do the trick. I've attached a pic of how it looks for your reference.

HarshKapadia2 commented 4 years ago

Also, you haven't handled the following point:

I don't agree with the variable names finger and peace as they can be confusing for someone. The previous variable names output_1 and output_2 were clearer.

Could you please keep the original variable names? (Please make a new commit for this.)

Haroonchik commented 4 years ago

@HarshKapadia2 Check.

HarshKapadia2 commented 4 years ago

The changes seem fine now! 👍 Thank you for the PR!

Please remember all the things that I told you when you make more PRs. Also, create a new branch for every PR, from a previous branch if the PRs are related or from the master if they are unrelated. Making PRs from the master is not a good practice.

Haroonchik commented 4 years ago

Thanks for the comments, they are very helpful. I will adhere to your recommendations