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

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

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

rwv
Copy link
Contributor

@rwv rwv commented Jan 11, 2023

@kbdharun kbdharun added enhancement clients It's a problem with a tldr client labels Jan 11, 2023
Copy link

@navarroaxel navarroaxel left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link

@EmilyGraceSeville7cf EmilyGraceSeville7cf left a comment

Choose a reason for hiding this comment

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

No fixes found, approved.
Thanks for a contribution! 🚀

@waldyrious
Copy link
Member

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!) 😅

@waldyrious waldyrious marked this pull request as draft January 11, 2023 15:27
@rwv
Copy link
Contributor Author

rwv commented Jan 11, 2023

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
Copy link
Contributor Author

rwv commented Jan 25, 2023

Any update? 😺 @waldyrious

@kbdharun
Copy link
Member

@waldyrious 👀

@waldyrious
Copy link
Member

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
  • Fig. 1: Current embed
  • Fig. 2: Current embed with the Patreon banner hidden — not that I think it shouldn't be there, but IMO it should take up less space and let more of the actual page content be displayed. Maybe it could be floated to the top right, below the GitHub stars button, or even instead of it.
  • Fig. 3: New embed
  • Fig. 4: New embed with a compressed header (quick experiment done with the browser's inspector — this is not an actual design proposal! 😅)

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

@rwv
Copy link
Contributor Author

rwv commented Feb 3, 2023

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
Copy link
Contributor Author

rwv commented Feb 3, 2023

@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
Copy link
Member

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 😁

@waldyrious waldyrious marked this pull request as ready for review February 3, 2023 20:16
@blueskyson
Copy link
Member

blueskyson commented Feb 4, 2023

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
Copy link
Contributor Author

rwv commented Feb 5, 2023

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

Copy link
Member

@waldyrious waldyrious left a comment

Choose a reason for hiding this comment

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

Ah, right, sorry for that — I meant to approve the PR with my previous comment, but I couldn't see the option in GitHub's mobile app, and then forgot to come do so in the desktop. LGTM!

@navarroaxel navarroaxel merged commit 19d40cb into tldr-pages:main Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clients It's a problem with a tldr client enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants