bitcoinxt / website

Website sources for bitcoinxt.software
6 stars 12 forks source link

Minor fixes #7

Closed ghost closed 9 years ago

ghost commented 9 years ago
ghost1542 commented 9 years ago

Except for the two issues detailed below, LGTM up to a1e2950.

H1 tags don't seem to have appropriate margin / text-align styling properties, which produces a ugly result:

capture du 2015-08-17 17 35 10

There is inconsistent margins between LI elements. This can probably be fixed by using margins instead of paddings, since margins should collapse.

capture du 2015-08-17 17 42 43

ghost commented 9 years ago

Should be better here: https://github.com/chmod755/website/commit/582044a8fe1f934ea112c4504a9d16e9b8295f2d

ghost1542 commented 9 years ago

@chmod755 Mmh, according to my testing last commit didn't fix those issues. Do you have access to building the website locally to preview your work?

Regarding the issue with titles, something like this would do the trick:

(H1 CSS selector) {
    ...
    margin: 1em 0;
    text-align: center;
}

Regarding the inconsistent paddings, you may need to dig a bit more...

Anyway, thanks for your work on this!

ghost1542 commented 9 years ago

Note; I just edited the CSS code sample as the previous version wouldn't work fine on the landing page.

ghost1542 commented 9 years ago

@chmod755 Oh, I just noticed the extra Welcome title on the landing page, perhaps we can keep it off? My first impression is that it doesn't add much to the content (but I'll leave it up to Mike to have the final word if he has a preference either way :) ).

ghost commented 9 years ago

I rarely use centered titles so I didn't think of that.

I don't see a difference between the padding on the current bitcoinxt.software website and my local version (compared it in FF 40.0 and Chromium 25)

"Welcome" can be removed or replaced if necessary

ghost1542 commented 9 years ago

@chmod755 Ah, you are right, sorry for the trouble. Your commit did fix the

  • paddings correctly; For some reason I had issues with my local preview.

    Re H1 titles: Yes centering them actually tends to work better on mobiles when icons are involved. The other thing I was submitting in my code sample was to set the same margins on H1 tags than the ones used by the H2 tags you have replaced, just to keep things aerated like before.

  • ghost1542 commented 9 years ago

    @chmod755 Do you have time to apply the remaining changes and fix the merge conflict? Please let me know, I can otherwise give you help and take care of adding the few required commits.

    mikehearn commented 9 years ago

    This was merged in via Saivann's pull - thanks!