-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Feature - Responsive Dottydoc (Part 1) - Main layout #2052
Feature - Responsive Dottydoc (Part 1) - Main layout #2052
Conversation
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.
Thanks for the PR @hhandoko!
I cloned your branch and generated the dotty site from it, technically most of it looks sound :)
I have a bit of feedback and questions before we merge this:
- The position of the hamburger icon has changed, we'd like to keep it where it was. Also in moving it, it looks like it lost its CSS styling
- The width of the sidebar now feels a bit too wide, what was your reasoning here?
- The animation from
☰
to˿
while nice-looking, doesn't really add anything in its context, I assume it's for mobile devices / something thinner than 767px - maybe it'd look ok with the old styling restored? Let's leave it in for the next iteration of the PR. - In the <767px view the size of the sidebar exposed is huge, I'm not sure if that was intentional or if it looks better on mobile. But we should probably differentiate between rules for mobile devices and smaller viewports on desktops, just a thought
As a side note - we're using the imperative mood for our commit messages. This means "Add X" instead of "Added X". Here's a quick primer:
|
Hi @felixmulder,
By CSS styling do you mean
I felt 250px was a bit too narrow as some package names got cut-off and most class names barely fit, so I added an extra 50px... But you have a good point, probably on narrower viewport width I can reduce it back to 250px.
I was looking to give more context with the left-pointing arrow, I've seen it implemented on the old Google Play Store mobile sidebar. Yes, the animation has no functional purpose, I just thought it looks nice 😁
Yes, it is intentional. As per earlier comments I'll reduce it back to 250px before minimising the sidebar at 576px. So to sum it up: Did I miss anything? Let me know if you have any further thoughts. Notes: |
I mean: vs before:
Not sure what you mean by "overlaps" in this sentence here is what it looks like on rMBP all up to date browsers:
I see, let's try to do our own thing 🙂
You're right, but then again, 300px is not enough for some projects, so this will be arbitrary - if we keep it at 250px - people can override the CSS on their own docs if need be. |
WRT the screenshot, it looks OK when the content body fits entirely in one page, but it becomes an issue on lengthy pages.
Yes, I'm not saying Dottydoc should copy ExDocs, just that I found that their UI pattern is useful and solves an existing problem given that the primary layout is very similar (as mentioned above).
Good point 👌 Let me know what you think about the menu positioning (above), and I'll update the layout 😄 Actions: |
@felixmulder, any thoughts? ☝️ |
Tablet (576px <= x < 768px) viewport sidebar is now limited to 250px to follow Desktop viewports whilst still retaining its off-canvas behaviour like Mobile. Mobile viewport (x < 576px) still implements 60% sidebar width.
Instead of specifying specific styles at a viewport width range, e.g. Tablet (576px <= x < 768px), the CSS properties are arranged as such that default CSS properties applies to Mobile, with Tablet and Desktop styles defined within media queries. Mobile-first strategy will ensure more consistency as style resolution will go in one direction, e.g. from Mobile -> Tablet -> Desktop.
The change above, in addition to qualifying `button#menu-toggle` with `div#content-wrapper` parent selector will make the existing CSS easier to refactor removing duplicates, and to swap it into CSS pre-processor languages such as SASS or Less.
Two different `div#content-body` styles was defined, one with only `position: relative` and another with the complete styles. In addition, a `div#content-body` Tablet and Desktop padding style was moved into the relevant media query to make future changes more transparent.
…h `0` Within the CSS file, RGBA property declaration is inconsistent: some with space after comma and some without, some has leading `0` for decimals and some dont'. This change make sure that one style is followed (space after comma and always use the leading `0` in decimals).
Hamburger menu now stays in the content body as per original design. However to prevent overlap with body text, a permanent 30px left padding on the body container has been added.
The last batch of commits completes the following: [X] - Add I've left default sidebar minimisation / collapse at 768px as before as the layout looks really awkward (576px <= x < 768px), as sidebar and content body is split almost equally down the middle. |
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.
Thoughts:
- Original style of hamburger menu still not restored
- Hamburger menu is still "fixed", not "absolute"
- Padding increased on content to match new style, not original
- We should probably default to closed menu on <768px as you said
Otherwise LGTM 🎉
As per PR review comments (scala#2052) the old hamburger menu style is restored by removing CSS transforms.
As per PR review comments (scala#2052) the hamburger menu positioning is changed back to absolute, which also means the left padding on content body is no longer needed and can be removed.
Thanks for the hard work @hhandoko! 🎉 |
Related ticket: #1945
Added better responsive support to sidebar by implementing off-canvas behaviour (i.e. content body slides to the right instead of becoming narrower).
Tested on: