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

Add a second screenshot of the page in dark mode #271

Closed
wants to merge 5 commits into from

Conversation

zoey-kaiser
Copy link
Contributor

Closes #270

This adds two different image versions to the ReadMe. One for Light and one for Dark Mode.

View the changes here: https://github.com/sidestream-tech/unified-auctions-ui/tree/add-dark-mode-image (switch your github from light to dark to see the changes)

One issue:

  • As we are now using the <image> element, we can no longer host the images in our own repository. Therefore I uploaded them to Imgur. Is this a good idea?

Comment on lines +2 to +3
<source media="(prefers-color-scheme: dark)" srcset="https://i.imgur.com/o7Oj6Rg.png">
<img alt="A quick preview of our UI" src="https://i.imgur.com/79FolWw.png">
Copy link
Contributor

Choose a reason for hiding this comment

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

This is nice! But I'm not sure if it's a good idea to store images outside of our repo, especially on the generic image hosting service like imgur:

Imgur keep images for ever as long as they are receiving at least 1 view every six months
https://www.quora.com/Imgur-How-long-are-the-images-stored-before-being-purged

Copy link
Contributor

Choose a reason for hiding this comment

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

@BracketJohn what's your take? Should we always keep images directly to the repo and potentially bloat its size over time? Or maybe use lfs for them (although I don't know if lfs will be displayed in the readme)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, we should not rely on external hosting of our images:

  • may be purged accidentally (I don't think 1 view every six month is a guarantee, rather a policy)
  • outage of external provider will result in (partial) outage of our service
  • external provider may apply any sort of alteration to terms & conditions at any time (e.g.: injecting some sort of user tracking into image serving?)

We already have a lot of parties we have to blindly trust (AWS, github, drone, google, ...) and should be careful with adding another one.

Especially if our problem is already solved via git-lfs.

@BracketJohn what's your take? Should we always keep images directly to the repo and potentially bloat its size over time? Or maybe use lfs for them (although I don't know if lfs will be displayed in the readme)?

It's possible to reference git lfs images in readmes or elsewhere: https://stackoverflow.com/a/47371057

My take: It's fine to add images to our repos, you can decide when to introduce lfs to make it more efficient. Here we are talking about 305kb, for me this would be too early to introduce lfs.

@zoey-kaiser
Copy link
Contributor Author

This PR is not needed atm, so I am going to close it. If we ever want to come back to this idea, it would be easy to implement!

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.

Add a second screenshot of the page in dark mode
3 participants