-
Notifications
You must be signed in to change notification settings - Fork 95
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
Fixed landing page design a bit (bootstrap errors, spacing, alignment). #242
base: master
Are you sure you want to change the base?
Conversation
site/index.html
Outdated
<div class="row"> | ||
<div class=" span12 col-sm-12 hidden-xs"><br><br></div> | ||
</div> | ||
<div class="row" style="display: flex; align-items: center"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I did 3 changes:
-
I added additional
<br>
. There was one before, now it is two. I added this<br>
and a couple of others below in some places in order to put the "get started" button close to the center of the page, and push Videos section closer to the bottom of it. I did this guided by the typical advice for designing a landing page -> you want your CTA to be the most attractive thing to click that is visible above the fold. In our case that is "getting started" button, potentially also purple "try" section. Videos section is still partially visible above the fold, but is a bit more down and therefore looks more like smth additional to explore than one of the main things to look at. -
I closed the div that is Bootstrap row here, instead of letting it also capture the stuff below. One Bootstrap row is always organized as 12 columns, but it was misused here, because somebody first added an element that spans all 12 columns, and then below one more element that spans 6, and then one more that spans 6!
The way Bootstrap handles such situation result in ok result, but it is semantically incorrect. So I closed this div here, so it contains only one element of 12 columns, and then opened a new one below (that is the last line). This element of 12 columns is anyway used to add vertical empty space in the whole width of the page, so it makes perfect sense that it is a standalone row.
This is merely a Bootstrap fix of the previous mistake somebody did. -
In the new "row" I created, I used the opportunity to align Haskell logo with the code example to the right by their centers. Before they were aligned by their tops. That wasn't that much of a bit deal before when there was no "get started" button, but now with that button added, height of haskell logo vs code example + get-started button has bigger difference than before, which resulted in a bit weird look in my opinion.
<br class="hidden-xs"> | ||
<img src="/img/haskell-logo.svg" class="img-responsive"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I merely formatted the html a bit.
<div class="row" id="code-example"> | ||
<div class="branding sample"> | ||
<br class="visible-xs visible-sm"> | ||
<h4 class="tag">Declarative, statically typed code.</h4> | ||
<div title="This example is contrived in order to demonstrate what Haskell looks like, including: (1) where syntax, (2) enumeration syntax, (3) pattern matching, (4) consing as an operator, (5) list comprehensions, (6) infix functions. Don't take it seriously as an efficient prime number generator." class="code-sample"> | ||
<pre><span class='hs-definition'>primes</span> <span class='hs-keyglyph'>=</span> <span class='hs-varid'>filterPrime</span> <span class='hs-keyglyph'>[</span><span class='hs-num'>2</span><span class='hs-keyglyph'>..</span><span class='hs-keyglyph'>]</span> | ||
<span class='hs-keyword'>where</span> <span class='hs-varid'>filterPrime</span> <span class='hs-layout'>(</span><span class='hs-varid'>p</span><span class='hs-conop'>:</span><span class='hs-varid'>xs</span><span class='hs-layout'>)</span> <span class='hs-keyglyph'>=</span> | ||
<span class='hs-varid'>p</span> <span class='hs-conop'>:</span> <span class='hs-varid'>filterPrime</span> <span class='hs-keyglyph'>[</span><span class='hs-varid'>x</span> <span class='hs-keyglyph'>|</span> <span class='hs-varid'>x</span> <span class='hs-keyglyph'><-</span> <span class='hs-varid'>xs</span><span class='hs-layout'>,</span> <span class='hs-varid'>x</span> <span class='hs-varop'>`mod`</span> <span class='hs-varid'>p</span> <span class='hs-varop'>/=</span> <span class='hs-num'>0</span><span class='hs-keyglyph'>]</span></pre> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I just added a bootstrap "row", in line 20, because it was missing, which is a mistake from before. Everything else is not a change, just indentation changed a bit so git thinks its new.
<br> | ||
<br class="hidden-sm"> | ||
<br class="hidden-sm"> | ||
<br class="hidden-xs hidden-sm"> | ||
<br class="hidden-xs hidden-sm"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I added some extra spacing, as explained above. So before it was 3 brs, now it is 5. Notice that just one happens on any screen size, other 2 happen only when screen is > sm, and other 2 happen only when screen > xs. So, smaller the screen, less spacing, which is what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of adding gaps via <br>
tags looks wrong to me. Combining multiple makes it worse.
In general, if the intention is to give a container "more room", I'd change it's padding / margin.
If the idea is to change the gap between containers, I'd suggest looking into the gap
property.
If one still ends up wanting a customized "spacer", I'd be more explicit, and define one. It's height can change depending on media queries. The also seems to be a $spacer
variable in bootstrap.
<br class="hidden-sm"> | ||
<br class="hidden-sm"> | ||
<br class="hidden-xs hidden-sm"> | ||
<br class="hidden-xs hidden-sm"> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as with spacings above.
Personally I'm not particularly keen on this. For me it's a question of "if it ain't broke, don't fix it" and I don't see that there's anything particularly broken here. I'd be interested to know what others think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a reasonable set of improvements to me. The small visible tweaks are an improvement, and it seems better to match with bootstrap's semantic model if we can.
Makes sense, although I would argue that in this instance it is broken, just hasn't manifested yet -> here I am referencing to incorrect usage of bootstrap rows. |
Yes, fixing the bootstrap thing makes more sense. I think it can afford to be separated into its own PR. |
38e7cfa
to
b6871fe
Compare
Ok, I opened a new one with just bootstrap stuff here: #243 . I adjusted this PR to be the same as the bootstrap one + 1 commit extra that brings landing page improvements via spacing -> so the same thing as it was, but changes it does are clearly delineated in commits (the second one is what this PR really does). If I could I would have made it to point to bootstrap PR but I can't because it is on my fork. And I didn't want to make it directly to master because changes I made in bootstrap PR are needed for this PR. |
Looks like some merge conflicts got into here after the force push. |
b6871fe
to
2c3606a
Compare
Thanks, my mistake, should be ok now. |
@tomjaguarpaw auf that is an issue indeed, thanks for this and I should have checked that. I fixed it now by removing the flexbox styling I did to align Haskell logo with the code on the right. I am not adamant that this PR needs to be merged -> I thought adding some spacing might be useful, but if you all feel it doesn't help much, I am ok with closing it, and somebody else can address the design in a better, more knowledgeable way, I am not very adept in design. |
I did a couple of things here at once:
<br>
s, in order to visually put more focus on "get started" button and on the "try" section, and to remove a bit of focus from the "videos" section and reduce visual clutter.Check out specific comments in the code for more details.
This is what the page looks like without this PR:
This is what it looks like with this PR: