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

Responsive view - 1 (testing / WIP) #1641

Closed
wants to merge 12 commits into from
Closed

Responsive view - 1 (testing / WIP) #1641

wants to merge 12 commits into from

Conversation

adsingh14
Copy link
Contributor

Changes

Issue: #1421

  • Add responsive view for both tablet and mobile
  • Add Scroll to Top button
  • Collapsible sidebar for small devices

Context

Current website is not responsive.

@adsingh14 adsingh14 changed the title Responsive view Responsive view - 1 Oct 21, 2021
@peff peff temporarily deployed to git-scm-pr-1641 October 23, 2021 23:03 Inactive
@peff
Copy link
Member

peff commented Oct 23, 2021

I lack the CSS experience to give this any kind of useful technical review. I can read it fine, and it follows about what I'd expect, but if there were gotchas or bad practices hiding, I'd have no clue. 😅

That said, it looks way better on mobile. So my inclination would be to give it a few days to see if anybody more intelligent than me shows up to give it review, but otherwise plan to merge.

One thing I did wonder: you added some new javascript, and we've generally tried to keep the page pretty functional even when the browser has javascript disabled. But I couldn't find any cases where it didn't seem to behave reasonably even with javascript disabled.

So...comments / review from others?

@peff
Copy link
Member

peff commented Oct 23, 2021

Oh, also: thank you very much for working on this!

@adsingh14
Copy link
Contributor Author

adsingh14 commented Oct 24, 2021

I just noticed my live PR and will make changes along with no-JS sidebar. :)

@peff peff temporarily deployed to git-scm-pr-1641 October 24, 2021 16:47 Inactive
@peff peff temporarily deployed to git-scm-pr-1641 October 24, 2021 18:31 Inactive
@sivaraam
Copy link
Member

That said, it looks way better on mobile.

First off I must concur. The website does look way way better on mobile. Thanks for working on this @adsingh14 !

Having said that, I must admit that my web dev knowledge is pretty limited too. So, no guarantees on thorough reviews here too, sorry. But I also did some usability testing on the deployed site. It seems to work well for the most part, which is a good thing.

I noticed three things which I wanted to point out.

  1. When browsing a man page, the "X last updated in vY.YY" info shown near the version menu starts losing the required space it needs when the width goes below 400px. If possible, it might be better to show it in a new line below the version menu for mobile views.
    Screen Shot 2021-10-25 at 00 04 15
  2. When browsing a man page, the "Topics" menu that's found near the version menu is not fully visible in mobile view. Only part of it is visible the rest gets hidden it seems. Screenshots:
    Screen Shot 2021-10-24 at 23 50 02
    Screen Shot 2021-10-24 at 23 50 17
  3. Search appears not to be working well in the deployed site. I'm not sure if that's something to be concerned about or not, though.

@adsingh14
Copy link
Contributor Author

@sivaraam I'm aware of your mentioned issue already. I'm getting some issues to run search offline and that's my top priority.

@adsingh14
Copy link
Contributor Author

adsingh14 commented Oct 24, 2021

When browsing a man page, the "X last updated in vY.YY" info shown near the version menu starts losing the required space it needs when the width goes below 400px. If possible, it might be better to show it in a new line below the version menu for mobile views

May I know on which device you're testing this ?
Well, 480px is suitable width for extra-small devices.

image

@peff peff temporarily deployed to git-scm-pr-1641 October 24, 2021 20:53 Inactive
@peff peff temporarily deployed to git-scm-pr-1641 October 24, 2021 20:58 Inactive
@peff peff temporarily deployed to git-scm-pr-1641 October 24, 2021 21:54 Inactive
@adsingh14 adsingh14 closed this Oct 26, 2021
@adsingh14 adsingh14 changed the title Responsive view - 1 Responsive view - 1 (testing / WIP) Oct 26, 2021
@adsingh14 adsingh14 reopened this Oct 26, 2021
@adsingh14
Copy link
Contributor Author

@peff

...we've generally tried to keep the page pretty functional even when the browser has javascript disabled.

Great! I've added a few lines to run collapsible sidebar without javascript.

Before squashing my commits into a single one, is it mandatory to run 'searchbar' (refer. to #1421 (comment) )? Although, searched results screen is same as the other pages.

PS: I'll create a new branch which includes all changes in a single commit including 'No javascript' sidebar functionality.

@peff peff temporarily deployed to git-scm-pr-1641 October 26, 2021 18:52 Inactive
@peff
Copy link
Member

peff commented Oct 26, 2021

I haven't had a chance to look carefully at the new changes, but I re-spun-up the review app to give people something to look at. And I think the issue with search is unrelated to your PR. IIRC, the review-app setup doesn't have access to all of the secret variables, which includes the credentials for the search database. On a similar review-app which touches nothing related (https://git-scm-pr-1630.herokuapp.com/), search doesn't work there either.

@adsingh14
Copy link
Contributor Author

the review-app setup doesn't have access to all ... IIRC, the review-app setup doesn't have access to all of the secret variables, which includes the credentials for the search database.

Great then ! I will create a new PR which will have a single commit of the changes I made till now. :)

@adsingh14 adsingh14 closed this Oct 26, 2021
@adsingh14
Copy link
Contributor Author

Transferred to #1644.

@adsingh14 adsingh14 deleted the responsive-view branch November 11, 2021 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants