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.

Sign up for GitHub

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

Jump to bottom

Styling Tweak #56

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open

Styling Tweak #56

wants to merge 31 commits into from
+164 −130

Conversation

Spazcool
Copy link
Contributor

@Spazcool Spazcool commented Dec 19, 2017 •
edited
Loading

Removed forced upper & lower casing, nesting, margin and padding. Increased img size. Added link to img. #48 #39 #19

NEW STYLE:
style

OLD STYLE:
styleold

@lenzai lenzai requested a review from mattermoran December 19, 2017 23:51
@lenzai
Copy link
Member

lenzai commented Dec 20, 2017

@mattermoran you are the most qualified to review this code.
Please follow 2 distinct steps:

  • reviewing coding practices
  • suggesting design improvements.

@mattermoran
Copy link
Contributor

Good job, @Spazcool . Definitely looks better than before. Here's a little 'sketch' of what I think is not good. Not changing design or anything just tiny fixes that would make the page look better. Especially paddings should the same everywhere.
P.S. I personally don't think the grey background looks good. I would just leave it white.
wechatimg78

@Spazcool
Copy link
Contributor Author

Now with more consistent padding!
stylenwe

I tried without background colors, it bothered me to my very core.

@lenzai
Copy link
Member

lenzai commented Dec 20, 2017 •
edited
Loading

  1. padding between similar Items disappeared.
  2. Missing fix on padding below breadcrumbs
  3. double rounding on both the container and the picture is too much.
  4. new grey color is getting heavy and affecting the contrast of the text.

@lenzai
Copy link
Member

lenzai commented Dec 20, 2017

screen shot 2017-12-21 at 12 43 51 am
screen shot 2017-12-21 at 12 43 44 am

  1. huge wrench next to tiny title does not make sens.
  2. similar item, centered when there is only one looks weird, maybe align left?

@Spazcool
Copy link
Contributor Author

@lenzai @mattermoran I believe I've tackled all the issues you listed above. I'm not sure I like the way the breadcrumbs look with the extra padding, especially on the search page, but I defer to your guys' expertise.

Padding between similar items:
simstyle

Wrench & search page:
wrenchstyle

@Spazcool Spazcool mentioned this pull request Dec 21, 2017
Open
@lenzai
Copy link
Member

lenzai commented Dec 22, 2017 •
edited
Loading

how about something like that for wrench and picture?
screen shot 2017-12-22 at 10 30 31 am

@Spazcool
Copy link
Contributor Author

@lenzai your posted img doesn't look all that different from the old style, minus the margins. Do you want the img to be big or small?

oldimgwrench

@lenzai
Copy link
Member

lenzai commented Dec 22, 2017

Picture roughly same size as the container box. Text for title on one line

@Spazcool
Copy link
Contributor Author

It looks cleaner. Side by side your img is nicer, but again its a small img that will be viewed on a mobile. Do we want big imgs or streamlined item cards?

@Spazcool Spazcool assigned Spazcool and unassigned Spazcool Dec 23, 2017
@Spazcool
Copy link
Contributor Author

My attempts at incorporating Alex's style.

bread
search

@lenzai
Copy link
Member

lenzai commented Dec 23, 2017

I want the image to be roughly the same size as the container.

What you call the "old style" is an image that barely use half the the height of the container. Seems like extremely inefficient use the screen real estate. Once this design/coding issue is solved, we can afford to make the container fit roughly the same height as the text because we have confidence that we won't have 10px height picture as consequence.

I hope Alex can guide you through best practices to achieve that with minimalistic code.

@Spazcool
Copy link
Contributor Author

Without the background color:
whiteitem

whitesearch

@lenzai
Copy link
Member

lenzai commented Dec 27, 2017

Some kind of background for title area is fine.
Main article with white background and low padding looks nice. Video and I frame should also benefit from reduced padding.

lenzai
lenzai reviewed Dec 27, 2017
View reviewed changes
Copy link
Member

@lenzai lenzai left a comment

Choose a reason for hiding this comment

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

so we got rid of bootstrap and replace with many new classes.

a few div nesting disappeared.. Great!
But what else do we get in term of code readability and maintainability?

i_love_qr.svg
@@ -1 +0,0 @@
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 23 23"><path d="M1 1h7v7h-7zM11 1h2v2h1v1h-1v1h1v6h-1v-2h-1v-1h1v-2h-1v-3h-1v3h-1v3h-1v-6h1v-1h1zM15 1h7v7h-7zM2 2v5h5v-5zM16 2v5h5v-5zM3 3h3v3h-3zM17 3h3v3h-3zM11 7h1v1h-1zM1 9h1v1h-1zM3 9h5v1h-2v1h3v-1h1v-1h2v2h-2v1h-5v-2h-1v1h-1zM15 9h5v3h1v1h-3v-1h1v-2h-4zM21 10h1v1h-1zM1 11h1v1h-1zM12 11h1v1h-1zM14 11h1v3h-1zM3 12h2v2h-4v-1h2zM16 12h1v2h2v1h-2v1h-2v-1h1zM6 13h3v1h-3zM10 13h3v1h1v5h-1v-2h-2v-1h2v-1h-1v-1h-1v2h-1v-1h-1v-1h1zM21 13h1v1h-1zM1 15h7v7h-7zM19 15h2v1h-1v1h-2v-1h1zM2 16v5h5v-5zM9 16h1v3h1v1h-2zM21 16h1v2h-1zM3 17h3v3h-3zM15 17h1v1h-1zM11 18h1v1h-1zM16 18h3v2h1v1h-1v1h-1v-1h-2v-1h2v-1h-2zM12 19h1v1h-1zM14 19h1v1h-1zM9 21h2v1h-2zM12 21h1v1h-1zM20 21h1v1h-1z"/></svg>
Copy link
Member

Choose a reason for hiding this comment

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

off topic

views/partials/itemCards.ejs
<span> <a href="/search?floor=<%- floor %>"> <%- floor.toLowerCase() %> </a></span> >
<span> <a href="/search?floor=<%- floor %>&room=<%- room %>"> <%- room.toLowerCase() %> </a></span> >
<span> <a href="/search?floor=<%- floor %>&room=<%- room %>&location=<%- location %>"> <%- location.toLowerCase() %> </a></span> >
<span> <a href="/search?fixture=<%- fixture %>"> <%- fixture.toLowerCase() %> </a></span>
Copy link
Member

Choose a reason for hiding this comment

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

do we still need span?

views/recent.ejs
@@ -1,7 +1,7 @@
<% include ./partials/header %>
<div class="resultContainer col-xs-12">
<h1>Recently scanned QR code</h1>
Copy link
Member

Choose a reason for hiding this comment

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

why so picky about custom styling?

views/recent.ejs
@@ -1,7 +1,7 @@
<% include ./partials/header %>
<div class="resultContainer col-xs-12">
<h1>Recently scanned QR code</h1>
<article class="col-xs-12">
Copy link
Member

Choose a reason for hiding this comment

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

what does "sub" means?

@lenzai
Copy link
Member

lenzai commented Dec 27, 2017

@mattermoran is this what you recommend in term of optimal CSS/class coding?
Forget about graphic design for a while and let's focus on code simplicity

lenzai
lenzai reviewed Dec 27, 2017
View reviewed changes
Copy link
Member

@lenzai lenzai left a comment

Choose a reason for hiding this comment

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

Let @mattermoran review CSS/HTML code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lenzai lenzai lenzai left review comments

@mattermoran mattermoran Awaiting requested review from mattermoran

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Spazcool @lenzai @mattermoran