adobe / brackets.io

brackets.io website
111 stars 80 forks source link

New Design #118

Closed larz0 closed 10 years ago

larz0 commented 10 years ago

Feel free to review @MarcelGerber @le717.

Ignore blog.html. That's just an early wp blog template I'm working on.

Mark-Simulacrum commented 10 years ago

The menu bar seems to have an incorrect font for the text.

le717 commented 10 years ago

@Mark-Simulacrum Can you explain that a bit? The menu bar is using Source Sans Pro, just like the rest of the site, just in a different weight.

Mark-Simulacrum commented 10 years ago

@larz0 knows better than I, but to me the menu bar looks better without the special font of 400 15px / 50px 'Source Sans Pro' applied.

Mark-Simulacrum commented 10 years ago

In the code for retrieving starter issues and features, it adds the amount of comments on that issue. Is there a reason for this? Seems unnecessary to me.

le717 commented 10 years ago

@larz0 Here's what I'm seeing, not including the few comments I already left.

Overall, I really like it! :+1:

Mark-Simulacrum commented 10 years ago

The Javascript code on the contribute page fetches 7 starter features, but only 6 starter issues. Should this be changed within this PR, or should I open my own PR to fix that design oddity?

le717 commented 10 years ago

@Mark-Simulacrum

I don't see a reason for using a playlist here.

Why not? It allows you to watch many videos showing Brackets in action. AFAIK, Brackets does not have a current "this is it!" feature/overview video.

In the code for retrieving starter issues and features, it adds the amount of comments on that issue. Is there a reason for this? Seems unnecessary to me.

I agree, that's a bit TMI. It doesn't really add anything to the listings. It's just a few lines that can be taken out if desired.

The Javascript code on the contribute page fetches 7 starter features, but only 6 starter issues.

@larz0 can make the starter issues load 7 items. It's very simple to change &per_page=6 to &per_page=7. :wink:

Mark-Simulacrum commented 10 years ago

@le717

Why not? It allows you to watch many videos showing Brackets in action. AFAIK, Brackets does not have a current "this is it!" feature/overview video.

When using a playlist, the user may think that all of those videos pertain to the selected topic: which is not the case. A minor thing, though.

I agree, that's a bit TMI. It doesn't really add anything to the listings. It's just a few lines that can be taken out if desired.

Would opening a PR targeting master be the best way to resolve that?

The Javascript code on the contribute page fetches 7 starter features, but only 6 starter issues.

@larz0 can make the startwe issues load 7 items. It's very simple to change &per_page=6 to &per_page=7. :wink:

Great to hear!

larz0 commented 10 years ago

@Mark-Simulacrum the comments on the issue and feature cards are only there to show the levels of activity.

Mark-Simulacrum commented 10 years ago

@larz0 I see, although it still seems superfluous, since the user can easily retrieve this by clicking on the card. Perhaps a better alternative would be to ask for the most recent update. What do you think?

larz0 commented 10 years ago

@Mark-Simulacrum your concern is legitimate, I've thought about it as well and that's why I put them at the end of the card. I think it's useful to see which issues or features are being neglected (0 comments) and which ones are more active (controversial? important?).

Mark-Simulacrum commented 10 years ago

@larz0 I see your point. I'll think about this, not sure what the best solution is.

larz0 commented 10 years ago

@le717

larz0 commented 10 years ago

@Mark-Simulacrum @le717 thanks for feedback guys :+1:

njx commented 10 years ago

Looking good. Some small comments:

marcelgerber commented 10 years ago

The body text seems just a tad too small/light to be easy to read (on retina).

I totally agree. It's not too great on my 1366x768 15.6" LCD either. The contrast could be better as well, as grey-on-grey text isn't the best choice.

le717 commented 10 years ago

@njx On the topic of the screenshot, it might be best to take #117 into account for updating it.

EDIT

The body text seems just a tad too small/light to be easy to read (on retina).

I'm inclined to agree.It is a bit hard to read on my 1333x768 laptop screen as well, especially on text-heavy pages.

marcelgerber commented 10 years ago

@larz0 Guess you can spot the issue (happens only in the German version of the page): image

larz0 commented 10 years ago

@njx @MarcelGerber @le717

screen-shot-2014-10-21-at-9 51 32-am

njx commented 10 years ago

@larz0

I can change the screenshot. Should we just show the inline color picker? I'd like to use a different theme as well so the colors match the website more.

Yeah, I think showing it in non-split-view would work. I'm a little torn about whether we should show the color picker or the inline CSS editor - the latter is more unique, but the former is visually more appealing.

Hmm I'm actually trying to center the menu items vertically. If I make them line up with the baseline of "Brackets" then they won't be vertically centered with the header and I'd like the menu items to have a smaller font size.

Yup, I figured that's what you were trying to do, but because the size difference is small (and the item it's being centered relative to doesn't have a strong visual boundary, like a search box - the icon is a little too far away), it doesn't really look centered, just misaligned, IMO. If the Brackets logo/text were larger, I think it would work better.

I can tweak the body font weight or color a bit more but I won't change the font size in order to have an optimal measure.

I think tweaking the weight would help.

TomMalbran commented 10 years ago

The new design looks nice. But I really like the current header blue background. Do you think that we could bring it back? It gives a lot of color to the web and makes it look warmer. Now is a bit cold with so much white and black.

One other minor thing. I noticed that you replaced the font property a lot, repeating the font-family. But the replaced font-family doesn't add the fallbacks. I think that is better to not use the font property to change the font size and weight and use the longer font-size and font-weight properties for that.

larz0 commented 10 years ago

@njx I'll try the inline editor instead of the color picker. I've also pushed the logo up by 1px; seems a tad better.

@TomMalbran sorry that the current hero bg isn't your favorite, it's the style I'm going to go for right now but maybe we'll bring it back for v2. The sections below the hero are light blue and dark blue so it's not completely grayscale.

I'll add the font fallbacks in the font shorthand (nice catch). Once I have time I'll convert these to SASS or LESS so we can use variables.

larz0 commented 10 years ago

@MarcelGerber where are you seeing:

image

Could you take a screenshot of the page to give me more context.

marcelgerber commented 10 years ago

It's "Setup A Dev Environment" on the English contribute page. Hope that helps.

larz0 commented 10 years ago

@MarcelGerber I can't reproduce it. Do I need to switch to german? Screenshot?

marcelgerber commented 10 years ago

image What I did:

  1. Append ?lang=de to the URL (use ?lang=en afterwards to revert that change)
  2. Resize browser window to half-screen
larz0 commented 10 years ago

@MarcelGerber thanks. It should be fixed now :+1:

TomMalbran commented 10 years ago

Should we add waffle in the links?

Edit: It might be better to show the last 8 blog posts instead of 10.

Edit2: Shouldn't the image use the default theme and maybe no extra extensions?

larz0 commented 10 years ago
marcelgerber commented 10 years ago

The OS Alert looks odd.

larz0 commented 10 years ago

@MarcelGerber how do I see that alert?

marcelgerber commented 10 years ago

(In Chrome) Open DevTools, toggle the phone icon in the DevTools nav bar, then in the field "UA", input something like "Linux 10.04," (you have to enter that trailing comma!) and reload the page.

larz0 commented 10 years ago

@TomMalbran looks like the recent blog posts count is being controlled by wordpress?

larz0 commented 10 years ago

@MarcelGerber I've fixed the alert. Thanks.

marcelgerber commented 10 years ago

@larz0 See #120.

marcelgerber commented 10 years ago

screenshot_2014-10-22-22-12-40 The contributor avatars look a bit out of place on mobile. Guess we shouldn't show/load them while on mobile.

larz0 commented 10 years ago

@MarcelGerber hmm let me think about that.