Open core-ai-bot opened 3 years ago
Comment by Mark-Simulacrum Monday Oct 20, 2014 at 23:24 GMT
The menu bar seems to have an incorrect font for the text.
Comment by le717 Monday Oct 20, 2014 at 23:26 GMT
@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.
Comment by Mark-Simulacrum Monday Oct 20, 2014 at 23:28 GMT
@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.
Comment by Mark-Simulacrum Monday Oct 20, 2014 at 23:38 GMT
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.
Comment by le717 Monday Oct 20, 2014 at 23:39 GMT
@larz0 Here's what I'm seeing, not including the few comments I already left.
style.css
and brackets.io.css
, but the former is only applied on the blog page. Could it be possible to merge the two files and add a large comment divider saying something like "This is for the blog page.".selector { property: value; }
). Doing so reduces whitespace, and in turn file and download size._blank
?-webkit-filter
, the effect is lost in non-Webkit browser and everything is in color. However, I thought the color added to the page. It's kinda... blah in grayscale.Overall, I really like it! :+1:
Comment by Mark-Simulacrum Monday Oct 20, 2014 at 23:41 GMT
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?
Comment by le717 Monday Oct 20, 2014 at 23:48 GMT
@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:
Comment by Mark-Simulacrum Monday Oct 20, 2014 at 23:52 GMT
@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!
Comment by larz0 Tuesday Oct 21, 2014 at 04:02 GMT
@Mark-Simulacrum the comments on the issue and feature cards are only there to show the levels of activity.
Comment by Mark-Simulacrum Tuesday Oct 21, 2014 at 04:04 GMT
@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?
Comment by larz0 Tuesday Oct 21, 2014 at 04:12 GMT
@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?).
Comment by Mark-Simulacrum Tuesday Oct 21, 2014 at 04:13 GMT
@larz0 I see your point. I'll think about this, not sure what the best solution is.
Comment by larz0 Tuesday Oct 21, 2014 at 04:20 GMT
@le717
Comment by larz0 Tuesday Oct 21, 2014 at 04:21 GMT
@Mark-Simulacrum @le717 thanks for feedback guys :+1:
Comment by njx Tuesday Oct 21, 2014 at 16:44 GMT
Looking good. Some small comments:
Comment by MarcelGerber Tuesday Oct 21, 2014 at 16:52 GMT
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.
Comment by le717 Tuesday Oct 21, 2014 at 16:54 GMT
@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.
Comment by MarcelGerber Tuesday Oct 21, 2014 at 16:55 GMT
@larz0 Guess you can spot the issue (happens only in the German version of the page):
Comment by larz0 Tuesday Oct 21, 2014 at 17:02 GMT
@njx @MarcelGerber @le717
Comment by njx Tuesday Oct 21, 2014 at 17:15 GMT
@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.
Comment by TomMalbran Tuesday Oct 21, 2014 at 17:30 GMT
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.
Comment by larz0 Tuesday Oct 21, 2014 at 19:32 GMT
@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.
Comment by larz0 Tuesday Oct 21, 2014 at 19:40 GMT
@MarcelGerber where are you seeing:
Could you take a screenshot of the page to give me more context.
Comment by MarcelGerber Tuesday Oct 21, 2014 at 19:44 GMT
It's "Setup A Dev Environment" on the English contribute page. Hope that helps.
Comment by larz0 Tuesday Oct 21, 2014 at 19:48 GMT
@MarcelGerber I can't reproduce it. Do I need to switch to german? Screenshot?
Comment by MarcelGerber Tuesday Oct 21, 2014 at 19:58 GMT
What I did:
?lang=de
to the URL (use ?lang=en
afterwards to revert that change)Comment by larz0 Tuesday Oct 21, 2014 at 21:02 GMT
@MarcelGerber thanks. It should be fixed now :+1:
Comment by TomMalbran Wednesday Oct 22, 2014 at 17:25 GMT
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?
Comment by larz0 Wednesday Oct 22, 2014 at 17:39 GMT
Issue by larz0 Monday Oct 20, 2014 at 22:59 GMT Originally opened as https://github.com/adobe/brackets.io/pull/118
Feel free to review @MarcelGerber @le717.
Ignore blog.html. That's just an early wp blog template I'm working on.
larz0 included the following code: https://github.com/adobe/brackets.io/pull/118/commits