Skip to content

Conversation

@kidonng
Copy link
Contributor

@kidonng kidonng commented Sep 7, 2021

Before:

image

After:

image

@vercel
Copy link

vercel bot commented Sep 7, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/enix/gitako/831Zkq9gpu3SYeFAwVLjqvZnjyfg
✅ Preview: https://gitako-git-fork-kidonng-patch-1-enix.vercel.app

@EnixCoda
Copy link
Owner

EnixCoda commented Sep 7, 2021

Thank you! But I need to ask few questions before merging it.

  1. What is the motivation behind hiding the icon?
  2. What if the sidebar is expanded, should the sidebar be hidden? If not, why?
  3. Should other buttons added by gitako be hidden? If not, why?
  4. What if the user would like Gitako to be printed as well?

Overall, temporarily disabling Gitako makes more sense to me. Hope to learn about your thoughts behind it.

@kidonng
Copy link
Contributor Author

kidonng commented Sep 8, 2021

  1. What is the motivation behind hiding the icon?

On Chromium-based browsers you can use print to save a page as PDF. Gitako switch doesn't make sense on the saved PDF file. GitHub itself also have some @media print styles to hide unnecessary sections such as footer.

  1. What if the sidebar is expanded, should the sidebar be hidden? If not, why?

No, because hiding the toggle is enough.

  1. Should other buttons added by gitako be hidden? If not, why?

No, because they won't look out of place like the toggle.

  1. What if the user would like Gitako to be printed as well?

I don't think there is a reason to.

Overall, temporarily disabling Gitako makes more sense to me.

Sure but this is just meaninglessly avoiding the problem when it can be solved via a single line of CSS. "you can just turn if off" here saves neither developers nor users' time.

@EnixCoda
Copy link
Owner

EnixCoda commented Sep 9, 2021

Thank you!

It does not matter whether it can get solved by one line of CSS or more. My point is that the changes should make sense to me and not look like a temporary hack for unusual usages.

I do not print web pages much. Could you share some scenarios where you'd do that? So that I can understand the purpose better and know how to help if necessary.

@kidonng
Copy link
Contributor Author

kidonng commented Sep 9, 2021

It does not matter whether it can get solved by one line of CSS or more. My point is that the changes should make sense to me and not look like a temporary hack for unusual usages.

"Unusual usages" sure, not a lot people know the trick. "Temporary hack" definitely not, this is the standard way and as mentioned above GitHub itself also does similar things.

I do not print web pages much. Could you share some scenarios where you'd do that? So that I can understand the purpose better and know how to help if necessary.

It is really just making use of the portability of PDF. Saving web page as HTML is uncertain and requires decent browser and often working Internet.

While I agree it's less frequent to print a GitHub page compared to a receipt, I use it for archiving purposes when I want to save content beyond just code (for example comments).

I can understand this is not something you, as the author, may need but rejecting such a trivial improvement does not make much sense either. I submitted a similar change to another project (machsix/Super-preloader#652) and they don't even bother to doubt it.

@EnixCoda
Copy link
Owner

EnixCoda commented Sep 9, 2021

I see. I know it might make you frustrated by getting blocked on contributing such a minor change.

Doubting because I did not understand the motivation quite well and wanted to know if there is more need to be considered than the switch. For example,

  1. Could the sidebar be included in the rule to prevent the hassle of unpinning it before printing, if the sidebar is pinned but should not be printed?
  2. Should the source code fold buttons disappear when printing? They are too close to line numbers and the page will be cleaner without them.
  3. Should the folded source code sections show up when printing? It might be too late if you want to see the content when it is already printed.

I need to see your thoughts on them to make me more convinced. I really appreciate your patience. :)

Please give me some time on thinking about the above questions before merging. Thanks.

@EnixCoda EnixCoda changed the base branch from develop to feature/optimize-for-print September 21, 2021 12:08
@EnixCoda EnixCoda merged commit b733ed1 into EnixCoda:feature/optimize-for-print Sep 21, 2021
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.

2 participants