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.

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

Design Updates and Fixes #225

Merged
merged 54 commits into from
Oct 15, 2016
Merged

Design Updates and Fixes #225

merged 54 commits into from
Oct 15, 2016

Conversation

skipjack
Copy link
Collaborator

@skipjack skipjack commented Oct 10, 2016

cc @KyleAMathews @bebraw @oliverturner

I'm opening this PR to include many of the design changes implemented by @KyleAMathews in #202 as well as updating the logo and applying some other tweaks and fixes to various areas of the site. Here's what's done so far:

  • Added Geomanist and Cabin
  • Cleaned up the layout a bit
  • Simplified color scheme using @jhnns logo colors
  • Sort out the license and bring @jhnns' webpack/media work in
  • Bring the mobile sidebar back to life
  • Finish any other design tweaks
  • Add favicon

@KyleAMathews let me know if you think I missed anything design-wise. I think we want to stay away from js/inline styles, at least for now, and stick with the modular-scale plugin. Thanks again for your work on #202.

@jhnns @bebraw hoping we can get the license sorted out here but I'm not really sure how to handle this.

Note: Please be aware that right now certain features will only work the dev-server but before we get this merged I'm hoping to have things fully functional (with @bebraw's interactive addition I believe we can).

…not sure if rh will play nicely with preact)
…not sure if rh will play nicely with preact)
@@ -24,9 +21,8 @@ html {
}

body {
font: 300 getFontSize(0) 'Open Sans', 'Century Gothic', sans-serif;
font: 300 getFontSize(0) 'Cabin', 'Century Gothic', sans-serif;

Choose a reason for hiding this comment

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

I'd suggest using 400 for body text not 300. < 400 gets a bit hard to read on many devices.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, I was actually only importing Cabin in 400 and 600 (with italic versions of each) but forgot to update this... changing momentarily.

@KyleAMathews
Copy link

I haven't tested it but can't see why react-headroom wouldn't work with Preact.

@skipjack
Copy link
Collaborator Author

@KyleAMathews yea I just noticed when npm installing react-headroom that a react addon was installed as well and I wasn't sure if that would play well with preact. I'm not too familiar with preact yet, or react-headroom for that matter but looking forward to playing around with both more.

@SpaceK33z
Copy link
Member

SpaceK33z commented Oct 14, 2016

@gregvenech, are you going to make more big changes? Asking because I want to test this out, but that would be a waste of time if more changes are coming ;).

@skipjack
Copy link
Collaborator Author

skipjack commented Oct 14, 2016

@SpaceK33z nope in my mind this branch is good to go. I want to push any other changes to new PRs (e.g. more sidebar work, landing page, etc.). The only problems currently with merging are:

  1. Not sure if we want to merge this before the license is tweaked.
  2. Everything works fine on the dev-server but images aren't working in production builds. Waiting for @bebraw to take a look.

Aside from that it's good to go and I think we should get it merged asap.

@TheLarkInn
Copy link
Member

Before we merge could we make one more change? That would involve adding our Open Collective banner. It's done via script tag. https://github.com/OpenCollective/OpenCollective/wiki/Website-banner

Would you be able to add this, if it trivial, if it is not, we can separate this into a separate PR and Issue and then merge this and cut a release.

@skipjack
Copy link
Collaborator Author

@TheLarkInn yea I can try tonight. I'm thinking maybe just on the landing page?

@TheLarkInn
Copy link
Member

TheLarkInn commented Oct 14, 2016

Well you know what, let me put it in a separate issue. If this is ready to go lets merge it.

@TheLarkInn
Copy link
Member

Added to separate issues #248 and we'll do a separate PR for it.

@skipjack
Copy link
Collaborator Author

skipjack commented Oct 14, 2016

@TheLarkInn sounds good, yea I mean if you guys are comfortable merging it regardless of those two things I mentioned then by all means. We could also tweak the license quickly for now by adding the language I mentioned before...

Except where otherwise stated, this code is:
...
The logo and icons images are under a different license which can be found here.

And come back to it later if necessary. I'm no expert with licensing so I was kind of defaulting to all of you on this.

@TheLarkInn
Copy link
Member

TheLarkInn commented Oct 14, 2016

I'm comfortable with this. @SpaceK33z is going to test later and then we can merge.

@bebraw
Copy link
Contributor

bebraw commented Oct 14, 2016

@gregvenech About assets, fastest fix might be to copy images/fonts below assets directly and then fix paths to point there. In this case you'll end up with a single copy clause.

Ideally asset pipeline should go through webpack entirely, but that's something to tackle separately.

@skipjack
Copy link
Collaborator Author

skipjack commented Oct 14, 2016

@bebraw @TheLarkInn sounds good 👍 . I will make the changes to the license and try the assets suggestion.

@SpaceK33z
Copy link
Member

SpaceK33z commented Oct 14, 2016

Tested it:

  • Could the header be more like Kyle's design? The larger font + active state are a lot better in that design.
  • Kyle's design has a nice active state in the sidebar. You previously said you're going to do more work on the sidebar, so will you also take this into account then?
  • The "Edit this page" link is blue, maybe change this to be the same as other links?
  • The headings are the same color as the body text. Is this on purpose? In Kyle's design they are darker, so it stands out more.
  • Woah the footer looks nice now!

I also asked a designer-friend (@indri-indri) to look at this, he suggested this enhancement:

Before:

webpack

After adding text-rendering: geometricPrecision; to .logo__text:

webpack

Note: this was tested with Chrome on macOS. On some browsers / OS's this has no effect.

@skipjack
Copy link
Collaborator Author

skipjack commented Oct 14, 2016

  1. Yea I can tweak the header, personally I like it more subtle but I see what you mean.
  2. Exactly, I would push this to a separate PR, I can add this note to that issue. I'm leaning towards a separate MobileSidebar component as @KyleAMathews had instead of using the current one for both.
  3. I've gone back and forth on that edit link, and I tried it both ways. Problem was it wasn't popping enough in that dark yellow color. Imo this link should stand out from the others.
  4. Yep I can definitely tweak the header color.

Will jump on these things and the other two in a few hours.

@skipjack
Copy link
Collaborator Author

skipjack commented Oct 14, 2016

Ok should be ready for another review.

Copy link
Member

@SpaceK33z SpaceK33z left a comment

Choose a reason for hiding this comment

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

Awesome. Tested it again and it looks good!

I've gone back and forth on that edit link, and I tried it both ways. Problem was it wasn't popping enough in that dark yellow color.

Fair enough :).

@bebraw bebraw merged commit 5b862e2 into develop Oct 15, 2016
@skipjack skipjack deleted the feature/fonts-n-styles branch October 15, 2016 16:17
sallar pushed a commit to sallar/webpack.js.org that referenced this pull request Oct 19, 2016
@jhnns
Copy link
Member

jhnns commented Oct 20, 2016

Regarding the logo in the header: What's your opinion on changing the header to the actual image (instead of using HTML text)? Font rendering can be very shitty depending on the OS, so as a designer I would definitely prefer to use the pre-rendered image for the banner :)

@SpaceK33z
Copy link
Member

@jhnns, you mean to add the text as paths instead of the real font, right? That would definitely be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants