-
Notifications
You must be signed in to change notification settings - Fork 153
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
Restructure looks of compare.html site #782
Conversation
Note that this diff doesn't change input form of other pages like index or bootstrap.
there was a lot of blank space in compare site. This change would help moving the content near to screen central.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely improves the readability.
Especially the submit button, which was easy to overlook previously.
</div> | ||
<form id="settings" action=""> | ||
<fieldset id="commits"> | ||
<legend>Commits</legend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could adjust the legend to eg "Select comparision" or "Choose commits to compare" ?
<label for="stats">Choose a comparison method:</label> | ||
<select id='stats' name="stat"> | ||
</select><br> | ||
<input type="submit" value="Submit" onclick="submit_settings(); return false;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd include the buttons in the fieldset so the box is drawn around all of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong preference. To be honest, I'm not a web dev. This PR is result of googling around
plus some web basic knowledge. I'd leave it to maintainer to decide.
@@ -2,6 +2,7 @@ | |||
<html> | |||
<head> | |||
<meta charset="utf-8"> | |||
<meta name="viewport" content="width=device-width, initial-scale=1.0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It always worked fairly okay for me already on mobile, what sort of improvement does this bring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is current page on mobile, we have to zoon in to view the page.
With this meta tag, we don't. MDN wrote something about this: https://developer.mozilla.org/en-US/docs/Learn/CSS/CSS_layout/Responsive_Design#The_viewport_meta_tag
So you should always include the above line of HTML in the head of your documents.
I personally frequently found myself looking for the submit button in the wrong place, so I'm glad to see this! |
This PR:
Closes #776.
Below are some comparison of old and new looks.
Old
Top
Bottom
New
Top
Bottom
On emulated phone mode:
Warning: big picture