TokTok / website

The TokTok website
https://toktok.ltd/
GNU General Public License v3.0
10 stars 55 forks source link

improved overall responsiveness of design #20

Closed cebe closed 7 years ago

cebe commented 7 years ago

mainly:

will take a look at the responsiveness of the PR table when this one and #18,#19 are merged.


This change is Reviewable

iphydf commented 7 years ago
:lgtm_strong:

Reviewed 4 of 4 files at r1. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

robinlinden commented 7 years ago

Reviewed 4 of 4 files at r1. Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


toktok/_includes/nav.html, line 3 at r1 (raw file):

<nav id="sidemenu">
  <ul class="nav tree">
    <h4>Navigation</h4>

I don't think we need to tell people that this is the navigation and having a

without <h1..3> preceding it will warn on most/all style checkers. :P

If you want to keep it, make it a different element, but I'd prefer it gone.


toktok/static/css/main.css, line 18 at r1 (raw file):

  /* give tables more space on mobile */
  #content table {
    margin: 0 -1.5em;

Now the table is all the way up against the left edge of the browser with a margin still there on the right. I think it looks a bit awkward without any space at all on the left. With this PR: https://i.imgur.com/w1wFPFo.png With the responsive margin change undone: https://i.imgur.com/JCS9PdG.png With a change in #content's margins <750px instead: https://i.imgur.com/3peGelb.png Changing #content's margins instead of the table's margins would also prevent the white block on the right (https://i.imgur.com/yoCfCTn.png) from covering up the table.


Comments from Reviewable

robinlinden commented 7 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


toktok/_includes/nav.html, line 3 at r1 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
I don't think we need to tell people that this is the navigation and having a

without preceding it will warn on most/all style checkers. :P If you want to keep it, make it a different element, but I'd prefer it gone.

Stupid reviewable. That was code. Not a literal <h4>. I didn't even close it. :<


Comments from Reviewable

cebe commented 7 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


toktok/_includes/nav.html, line 3 at r1 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
Stupid reviewable. That was code. Not a literal `

`. I didn't even close it. :<

I mainly added this for the mobile view, which starts with a list of links which are not directly obvious. We could hide it on bigger screens.


toktok/static/css/main.css, line 18 at r1 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
Now the table is all the way up against the left edge of the browser with a margin still there on the right. I think it looks a bit awkward without any space at all on the left. With this PR: https://i.imgur.com/w1wFPFo.png With the responsive margin change undone: https://i.imgur.com/JCS9PdG.png With a change in `#content`'s margins <750px instead: https://i.imgur.com/3peGelb.png Changing `#content`'s margins instead of the table's margins would also prevent the white block on the right (https://i.imgur.com/yoCfCTn.png) from covering up the table.

I made this for the table on the main page, which works fine with this. I'd like to tackle the pull request table after this has been merged. The reason to do this is to use the maximum of space for the table on really small screens like a phone. We could lower the breakpoint value even more though to do this only on really small width.


Comments from Reviewable

robinlinden commented 7 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


toktok/_includes/nav.html, line 3 at r1 (raw file):

Previously, cebe (Carsten Brandt) wrote…
I mainly added this for the mobile view, which starts with a list of links which are not directly obvious. We could hide it on bigger screens.

Hiding it on bigger screens would be nice, regardless, it shouldn't be a <h4>. An <h1>, or something else would be fine.

It also has to be moved to be before the <ul> as those should only contain <li>s.


toktok/static/css/main.css, line 18 at r1 (raw file):

Previously, cebe (Carsten Brandt) wrote…
I made this for the table on the main page, which works fine with this. I'd like to tackle the pull request table after this has been merged. The reason to do this is to use the maximum of space for the table on really small screens like a phone. We could lower the breakpoint value even more though to do this only on really small width.

Fair enough. Removing the margins entirely is probably a good idea on the smaller devices. Let's keep at least .5em of margin for as long as we can though.


Comments from Reviewable

cebe commented 7 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


toktok/_includes/nav.html, line 3 at r1 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
Hiding it on bigger screens would be nice, regardless, it shouldn't be a `

`. An `

`, or something else would be fine. It also has to be moved to be before the `
    ` as those should only contain `
  • `s.

Done. made it a <div> as it has no semantic meaning except making it more clear on mobile.

Also moved it out of the <ul> that was by accident :)


toktok/static/css/main.css, line 18 at r1 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
Fair enough. Removing the margins entirely is probably a good idea on the smaller devices. Let's keep at least .5em of margin for as long as we can though.

Done. It should only apply now on devices where the table does not fit on the screen anyway.


Comments from Reviewable

iphydf commented 7 years ago

:lgtm_cancel: I have no idea about CSS, so I'll leave the LGTMing to someone else. I've reviewed it anyway.


Reviewed 4 of 4 files at r2. Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

robinlinden commented 7 years ago
:lgtm_strong:

Reviewed 3 of 4 files at r2, 1 of 1 files at r3. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

iphydf commented 7 years ago

Reviewed 1 of 1 files at r3. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable