Skip to content
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

Add support for mobile screen sizes on Darkfish #1025

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MatheusRich
Copy link

@MatheusRich MatheusRich commented Jul 5, 2023

This adds support for reading the Darkfish-generated docs in mobile devices. I tried to keep the changes minimal, and the current layout was mostly preserved.

The most notable change is the navigation sidebar, which is now hidden by default on "small screens" (everything below 1024px). It can be toggled by the button on the top left corner. This button implements the ARIA pattern for a disclosure widget. The icon for the button was taken from Iconoir, which is licensed under the MIT license.

The design and some of the implementation were loosely inspired by the Elixir lang docs.

Screenshots

General look (before/after)

image image

Code blocks (before/after)

image image

Desktop view (before/after)

The most notable change here is that I limited the content size to about 1000px. Long lines are hard to read, so I added this little quality-of-life improvement for desktop users.

image image

The sidebar can also be closed on desktop:

image

Video

Screen.Recording.2023-07-05.at.19.59.33.mov

Testing

I wasn't sure if this change required tests. LMK if I need to add them.

@MatheusRich MatheusRich marked this pull request as ready for review July 5, 2023 23:01
@@ -0,0 +1,26 @@
<button id="navigation-toggle" class="navigation-toggle" aria-label="Toggle sidebar" aria-expanded="true" aria-controls="navigation">
<!--
Copyright (c) 2021 Luca Burgio
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the right way to attach the license.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, maybe we could use U+2630 (☰) as an icon.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or U+2261(≡)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this file does, but I added it for completeness.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this file does, but I added it for completeness.

Comment on lines +4 to +10
<nav id="navigation" role="navigation">
<div id="project-navigation">
<%= render '_sidebar_navigation.rhtml' %>

<%= render '_sidebar_search.rhtml' %>
</div>
</nav>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this needs more content, but I needed a sidebar on this page. Otherwise, it's harder to return to the Home page.

min-width: 340px;
font-size: 16px;
width: 100%;
max-width: 64em;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this could be even smaller.

@nobu
Copy link
Member

nobu commented Jul 6, 2023

Thank you, seems very nice.
Just one concern, when resizing the navigation pane, the menu icon doesn't move.

@MatheusRich
Copy link
Author

MatheusRich commented Jul 6, 2023

when resizing the navigation pane, the menu icon doesn't move.

@nobu, I totally missed that it is resizable! Would it be a problem if we make it fixed-sized? I think that would be a small sacrifice for a simpler solution.

Edit: I made the sidebar fixed-sized.

@MatheusRich MatheusRich requested a review from nobu July 11, 2023 13:19
@MatheusRich
Copy link
Author

@nobu could you approve the workflow so we can see if this works on CI? Thank you!

@MatheusRich
Copy link
Author

It looks like the tests breaking here are the same as the ones in the main branch.

@MatheusRich
Copy link
Author

@nobu @hsbt is there something I can do to move this forward?

@MatheusRich
Copy link
Author

@nobu @hsbt I've rebased this PR. CI should pass now.

@MatheusRich
Copy link
Author

Sorry to ping you again, @nobu. Can we get a second look at this?

@MatheusRich
Copy link
Author

cc @eregon @hsbt

@MatheusRich
Copy link
Author

@peterzhu2118 can you take a look at this?

This adds support for reading the Darkfish-generated docs in mobile devices.
I tried to keep the changes minimal, and the current layout was mostly
preserved.

The most notable change is the navigation sidebar, which is now hidden by
default on "small screens" (everything below 1024px). It can be toggled by
the button on the top left corner. This button implements the ARIA
pattern for a [disclosure widget]. The icon for the button was taken from
[Iconoir], which is licensed under the MIT license.

The design and some of the implementation were loosely inspired by the
[Elixir lang docs].

[disclosure widget]: https://www.w3.org/WAI/ARIA/apg/patterns/disclosure/
[Iconoir]: https://iconoir.com/
[Elixir docs]: https://hexdocs.pm/elixir/1.15.2/Kernel.html
@MatheusRich
Copy link
Author

@hsbt any plans on merging this (or not)?

@hsbt
Copy link
Member

hsbt commented Mar 18, 2024

I have no opinion that.

@nobu How about your opinion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants