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

Resp view #1644

Merged
merged 5 commits into from
Nov 11, 2021
Merged

Resp view #1644

merged 5 commits into from
Nov 11, 2021

Conversation

adsingh14
Copy link
Contributor

Changes

Issue: #1421

  • CSS fix
  • Add responsive view for both tablet and mobile
  • Add Scroll to Top button1
  • Collapsible sidebar for small devices1

Context

Current website is not responsive and mobile-ready.


Footnotes

  1. <noscript> functionality 2

@pedrorijo91 pedrorijo91 temporarily deployed to git-scm-pr-1644 October 27, 2021 19:07 Inactive
@peff peff temporarily deployed to git-scm-pr-1644 October 28, 2021 03:12 Inactive
@adsingh14
Copy link
Contributor Author

@peff
I hope you've checked the responsive view which doesn't break the functionality of website.

@peff peff temporarily deployed to git-scm-pr-1644 November 8, 2021 10:21 Inactive
@peff peff temporarily deployed to git-scm-pr-1644 November 8, 2021 10:48 Inactive
@peff
Copy link
Member

peff commented Nov 8, 2021

@adsingh14 Sorry it took so long for me to get back to this. The result looks pretty good to me!

A few things I noted:

  • there was a funky noop merge on your branch, as well as some small whitespace issues (trailing whitespace on lines noticed by git show --check, dropping newlines from end of text files, and one or two funny indentations). I fixed those up and force-pushed onto your branch, as it makes this and future diffs a little smaller. I hope that isn't disruptive to any ongoing work (if it is, you can force-push back over it, though I hope to just merge this soon anyway).
  • the <noscript> sidebar works fine for me if I disable javascript entirely, but not when using a blocker like ublock. AFAICT this is a ublock thing, and there's not much we can do about it on our end (it just doesn't respect the tags except for in a few cases). The non-js version of the sidebar works so well, though, it makes me wonder: what is the advantage of the js version? I.e., would it be worth just always using the no-js version?
  • I have mixed feelings on the "scroll to top" button, as it can sometimes cover actual content. It's not too bad, but I'm also not really sure what it's accomplishing. Thoughts on getting rid of it?

@adsingh14

This comment has been minimized.

@peff
Copy link
Member

peff commented Nov 8, 2021

I don't think so. Javascript actually decrease lines of code and helpful to create better UI/UX experience. CSS () part is just a tweak.

They seem identical to me in function, with the exception of the "X" to close in the javascript version. I do agree that fewer lines is nicer in general, but if we're carrying both anyway, that's more total lines (plus the seldom-used one will be subject to bitrot).

@adsingh14

This comment has been minimized.

@peff
Copy link
Member

peff commented Nov 8, 2021

Should I remove Javascript code for collapse-able sidebar and consider as a final change to get this merged?

I think that's my preference, but if you (or anybody) has a strong opinion, we could go the other way. As you noted, there are already parts of the site that don't work without javascript, so I'm not sure how common it is.

I'm not against the CSS but most of code runs best with JS which is supported by all web browsers (incl. IE).

My main concern here is people who disable javascript with extensions like ublock. It doesn't run the javascript, but also doesn't use the noscript bits, so you end up with the navbar disappearing but no way to bring it back.

I guess another alternative is to make the javascript more progressive: do the bits in noscript by default (without a noscript tag), and then if the javascript is run, have it mark up the DOM to do whatever it needs. That sounds like work, though, and I don't want to make you do more work.

@peff peff temporarily deployed to git-scm-pr-1644 November 8, 2021 19:32 Inactive
removed <noscript> from sidebar
@peff peff temporarily deployed to git-scm-pr-1644 November 9, 2021 08:01 Inactive
@adsingh14
Copy link
Contributor Author

@peff I keep CSS sidebar option and removed extra lines from .js and other files. 'Scroll-to-Top' <noscript> won't affect the core functionality. So, the recent change is more viable.

peff added 2 commits November 11, 2021 11:31
The previous commits stripped the trailing newline from several files.
Let's restore them to their canonical form to prevent noise in later
diffs.
These aren't hurting anything functional, but they may make future diffs
more noisy. Let's clean them up now.
@peff peff temporarily deployed to git-scm-pr-1644 November 11, 2021 16:35 Inactive
@peff
Copy link
Member

peff commented Nov 11, 2021

@adsingh14 I applied a few whitespace fixes up on top, but I think at this point we should merge and iterate further in new PRs if we turn up any problems. Thanks again for working on this!

@peff peff merged commit f27ee82 into git:main Nov 11, 2021
@adsingh14
Copy link
Contributor Author

@peff Thank you for whitespace fixes. May I know how to fix this issue for in future changes ? I've used VS Code for all changes.
I also checked the live website but nothing has changed there !

@peff
Copy link
Member

peff commented Nov 11, 2021

Thank you for whitespace fixes. May I know how to fix this issue for in future changes ? I've used VS Code for all changes.

I'm not sure how to prevent the "no newline at end of file" issues. Most editors I've seen include it by default; I'm not sure if there's some VS Code setting at work here (I don't use it myself).

For trailing whitespace on lines, you can view your commits with git show --check (or git diff --check before making a commit) to see which ones Git takes note of.

I also checked the live website but nothing has changed there !

Yeah, there's a big Cloudflare cache in front of the Heroku deployment, so changes may take up to 4 hours to be seen via https://git-scm.com. You can visit https://git-scm.herokuapp.com directly to see the deploy in the meantime (and indeed, it looks great, including the search results!).

@adsingh14 adsingh14 deleted the resp-view branch November 11, 2021 19:20
@sivaraam
Copy link
Member

sivaraam commented Nov 12, 2021

I'm not sure how to prevent the "no newline at end of file" issues. Most editors I've seen include it by default; I'm not sure if there's some VS Code setting at work here (I don't use it myself).

If it seems worth doing, we could consider adding editorconfig files for the repo which I perceive to be a universal solution for formatting requirements (new line at end of file, trimming trailing whitespaces). You could find more information regarding Editorconfig in their site: https://editorconfig.org/

My understanding is that it works by relying on built-in support in editors and existence of plug-ins where built-in support is lacking.

@adsingh14
Copy link
Contributor Author

I'm not sure how to prevent the "no newline at end of file" issues.

I used WSL Terminal for ruby based projects. To clean the CRLF mess, I cloned the repo, again, in WSL home folder. My recent commit(47ff215) is the result of 'no-headache' PR.

@adsingh14
Copy link
Contributor Author

My understanding is that it works by relying on built-in support in editors and existence of plug-ins where built-in is lacking.

On Windows, WSL Terminal works best to avoid extra packages/plugins. Instead of using Windows drive, use of home or Ubuntu directory is recommended for this project.

peff added a commit that referenced this pull request Jul 29, 2022
This was added in #1644 (making the site more responsive), but can
result in us breaking mid-word, which is ugly.

Fixes #1712.
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.

4 participants