tldr-pages / tldr-pages.github.io

🌐 The website for the tldr pages project
http://tldr.sh/
MIT License
76 stars 36 forks source link

Update live demo url to tldr.inbrowser.app #65

Closed rwv closed 1 year ago

rwv commented 1 year ago

See also https://github.com/tldr-pages/tldr/pull/9722

https://github.com/tldr-pages/tldr/issues/9727

waldyrious commented 1 year ago

I'll try to review this more carefully (as in, consider the implications, and ensure we make the related changes in sync) during the weekend. In the meantime, please allow me to mark this as a draft, as I did with #9722. I apologize for my anxiety as approvals pile up, even though that doesn't necessarily mean this will get merged hastily (but it might!) :sweat_smile:

rwv commented 1 year ago

I'll try to review this more carefully (as in, consider the implications, and ensure we make the related changes in sync) during the weekend.

@waldyrious Please feel free to contact me if you have any questions!

rwv commented 1 year ago

Any update? 😺 @waldyrious

kbdharun commented 1 year ago

@waldyrious πŸ‘€

waldyrious commented 1 year ago

Hi all! I'm so sorry for the delay in reviewing this! Please bear with me as I've got a bunch of things going on, at work and in my personal life.

So, overall I appreciate this change and am in favor of it. I would just propose a couple small tweaks before it's merged β€” specifically, I'd like there to be a way to reduce the embedded page's "chrome", i.e. display the meta information more compactly. This could either be done with CSS media queries to detect a constrained viewport, or with an URL parameter that could be passed to the app (something like https://tldr.inbrowser.app/pages/common/tar?embed=true, for example).

For reference, this is what I see in my browser (Firefox). Current embed:

image image
Fig. 1 Fig. 2
image image
Fig. 3 Fig. 4

What do you all think? Would such a change make sense?

rwv commented 1 year ago

What do you all think? Would such a change make sense?

I will implement this using ?tldr_embed=true and consider change layout not only in embed view but also mobile view.

rwv commented 1 year ago

@waldyrious Thank you for your wonderful suggestion! ❀️ However, I don’t think these suggestions block this PR and these suggestions can be issues in that repo too.

This PR was opened a month ago which makes me kind of unfocused. πŸ˜΅β€πŸ’«

waldyrious commented 1 year ago

You're right. Apologies again for the delay. I'm open to have the PR merged as-is β€” let's not make the perfect been the enemy of the good. But I do hope you be able to implement the change in the near future 😁

blueskyson commented 1 year ago

Fig 3. looks good to me. If a visitor wants to have a better experience, they will naturally go to the InBrowser App. We can merge it now and enhance the embedded version later.

rwv commented 1 year ago

@waldyrious @sbrl Can you review this PR? Thank you!