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

PAS-438 | Touch up the readme with more information #569

Merged
merged 3 commits into from
May 27, 2024
Merged

Conversation

Tyrrrz
Copy link
Contributor

@Tyrrrz Tyrrrz commented Apr 22, 2024

No description provided.

@Tyrrrz Tyrrrz requested a review from abergs April 22, 2024 18:50
@Tyrrrz Tyrrrz requested a review from a team as a code owner April 22, 2024 18:50
Copy link

codecov bot commented Apr 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 32.90%. Comparing base (a2ea77a) to head (31d57a4).
Report is 22 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #569      +/-   ##
==========================================
+ Coverage   32.56%   32.90%   +0.33%     
==========================================
  Files         525      518       -7     
  Lines       26533    26189     -344     
  Branches      858      774      -84     
==========================================
- Hits         8640     8617      -23     
+ Misses      17774    17448     -326     
- Partials      119      124       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

README.md Show resolved Hide resolved
@Tyrrrz Tyrrrz changed the title Touch up the readme with more information PAS-438 | Touch up the readme with more information Apr 24, 2024
README.md Outdated Show resolved Hide resolved
@Tyrrrz Tyrrrz enabled auto-merge (squash) April 24, 2024 20:25
@Tyrrrz
Copy link
Contributor Author

Tyrrrz commented May 13, 2024

@abergs @jonashendrickx any comments? 🙂

abergs
abergs previously approved these changes May 16, 2024
@abergs
Copy link
Member

abergs commented May 16, 2024

@Tyrrrz Could we perhaps include a link to our demo or a gif on the UX? I think it's powerful.

@Tyrrrz
Copy link
Contributor Author

Tyrrrz commented May 16, 2024

@Tyrrrz Could we perhaps include a link to our demo or a gif on the UX? I think it's powerful.

Do we have such a demo/gif already?

@Tyrrrz
Copy link
Contributor Author

Tyrrrz commented May 21, 2024

@abergs I added the demo video. There were two options on how to embed it into the readme:

  1. GIF. This is the most straight-forward option and supports auto-play. However, the file size is quite big -- 11mb. I can sacrifice the quality or the framerate to compress it, but then it looks quite ugly.
  2. Video element. It doesn't support auto-play. But it allows us to embed an actual video file, namely webm in this case, which is only 800kb in size without any loss in quality. The file is also not part of the repository but instead hosted by GitHub, so it won't affect cloning.

I went with option 2. This is what it looks like: https://github.com/bitwarden/passwordless-server/tree/Tyrrrz-patch-1?tab=readme-ov-file#demo

As a side note, I was able to record the Windows Hello popup without issues, so the user can see the whole authentication process.

Let me know if this is good enough.

@Tyrrrz Tyrrrz requested a review from abergs May 21, 2024 17:58
@abergs
Copy link
Member

abergs commented May 27, 2024

Solid! @Tyrrrz

@Tyrrrz Tyrrrz merged commit a8bbb9a into main May 27, 2024
15 checks passed
@Tyrrrz Tyrrrz deleted the Tyrrrz-patch-1 branch May 27, 2024 14:30
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.

None yet

3 participants