Skip to content
This repository has been archived by the owner on Dec 7, 2018. It is now read-only.

Design adjustments #225

Merged
merged 6 commits into from
Jul 10, 2017
Merged

Conversation

motleydev
Copy link
Contributor

@motleydev motleydev commented Jul 10, 2017

What:
I've made a series of design changes such as layout, margins and max-width settings.

Why:
The readability is not very good in its current state.

How:
Exclusively through style adjustments.

Before

screen shot 2017-07-10 at 20 58 05

After

screen shot 2017-07-10 at 20 59 48

My feelings won't be hurt if you hate my style preference! :) Thanks!

@codecov
Copy link

codecov bot commented Jul 10, 2017

Codecov Report

Merging #225 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #225   +/-   ##
=======================================
  Coverage   95.87%   95.87%           
=======================================
  Files          30       30           
  Lines         291      291           
  Branches       37       37           
=======================================
  Hits          279      279           
  Misses         12       12
Impacted Files Coverage Δ
components/code-preview.js 100% <ø> (ø) ⬆️
styles/base.js 100% <ø> (ø) ⬆️
components/docs-page.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c634781...f3696ec. Read the comment docs.

@kentcdodds
Copy link
Owner

This is great! Thank you so much for these contributions! Do you think you could follow the instructions for deploying a version of this for people to look at?

I was just thinking yesterday that the font was too thin, I think you may have made it too thick though, maybe somewhere in the middle? 😉

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Just some thoughts. I'd love to see a deployed version of this :)

styles/base.js Outdated
@@ -144,7 +141,7 @@ export default () => `
/* to be used by our code blocks */
.prism-code {
display: block;
white-space: pre;
white-space: pre-wrap !important;
Copy link
Owner

Choose a reason for hiding this comment

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

If !important is needed here, let's add a comment explaining why.

@@ -68,12 +71,12 @@ function PageSections({title, note, heading, sections}) {
<PageWrapper>
<Title dangerouslySetInnerHTML={{__html: mdToHTMLUnwrapped(title)}} />
<Div
maxWidth="70rem"
maxWidth="35rem"
Copy link
Owner

Choose a reason for hiding this comment

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

I feel that 35rem is a little too narrow... How about 50rem? I think that's where we had it at one time...

@motleydev
Copy link
Contributor Author

Your wish is my url link.

  • I'll wait for feedback after you've poked around the live version then I'll add the comment about stuff like the important!.
  • Sadly I'm not sure there is a lighter version of the font, that's already 400 and the next lower version in the family is 100.
  • 50em still seems to break the 50-60 char/line But I'm happy to oblige. :) Let me know what you think of it "live"

Do it Live

@kentcdodds
Copy link
Owner

Ok, the font actually looks great 👍 thanks!

I still think it's a bit too narrow. Especially on my 4k monitor 😅

screen shot 2017-07-10 at 2 41 22 pm

In all seriousness, it also makes the interactive code blocks pretty narrow:

screen shot 2017-07-10 at 2 41 26 pm

We take a lot of hints from styled-components.com and they're doing 56rem. I think that 50rem should be ok without causing any trouble for folks.

Thanks again! This is great 👍

@@ -144,8 +141,10 @@ export default () => `
/* to be used by our code blocks */
.prism-code {
display: block;
white-space: pre;

white-space: pre-wrap !important;
Copy link
Owner

Choose a reason for hiding this comment

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

These styles in particular are borrowed from react-live, which injects them again. We could probably tell react-live to not inject the styles. But this is ok for now.

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This is great, thanks again!

@kentcdodds kentcdodds merged commit acda340 into kentcdodds:master Jul 10, 2017
@kentcdodds
Copy link
Owner

Would you like to add yourself to the contributors table?

@motleydev
Copy link
Contributor Author

NP! Happy to help! I've learned a lot from watching your Egghead videos so I'm stoked to be able to give a little back!

@kentcdodds
Copy link
Owner

Haha, @motleydev, could you fix this on the advanced page please? 😂

screen shot 2017-07-11 at 9 19 27 pm

@kentcdodds
Copy link
Owner

Those are shields.io badges and should not have a width of 100% 😄

@motleydev motleydev deleted the design-adjustments branch July 12, 2017 07:00
@motleydev
Copy link
Contributor Author

motleydev commented Jul 12, 2017

Just sat down to do this but looks like @tdeschryver just beat me to it with #231

@timdeschryver
Copy link
Collaborator

Oh sorry, I didn't see this 😄

@kentcdodds
Copy link
Owner

That went pretty quick! You seem to be interested in design! Maybe you can help with this! #219

@motleydev
Copy link
Contributor Author

Love to! Heading in to take a look.

@motleydev
Copy link
Contributor Author

Wrapping up my day here in DE. I'm going to have to take a look at this tomorrow.

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

Successfully merging this pull request may close these issues.

3 participants