-
Notifications
You must be signed in to change notification settings - Fork 208
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
fix(topbar): support teams in topbar owner links #2432
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome work, thank you for taking care of it!
I had some comments inline, and I also did a manual test:

I think while the link is correct now, we should render the team differently.
Perhaps a different icon, and shorten the team-name?
I'm trying to remember what the github way of rendering is, perhaps something like @rustcrypto/block-chipers
instead of github:rustcrypto:block-ciphers
?
If needed, I can fix the GUI in a later PR. |
eda029c
to
4addf4c
Compare
@rustbot ready |
@rustbot label -S-waiting-on-author +S-waiting-on-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your continued work on this!
last missing piece would be a small test for the new behaviour in the topbar.
( if you don't have time for this I can also approve & merge this and add the test myself, as the current behaviour is also not tested :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did another manual test of this PR and decided to merge it as-is.
In separate PRs I would recommend that we:
- add a test
- render teams a little differently.
Currently the owner links in the top bar does not support teams. This pull request updates the database to store the
kind
of the crate owner (currently eitheruser
orteam
). The owner kind is added or updated to the database when a crate is built. It also updates the top bar to render the appropriate link based onkind
.Finally, this pull request adds a subcommand to update all crate data from thecrates.io
API.I chose not to use an enum for thekind
of the crate owner because the only place in the code which we directly need to make a decision based on the crate owner kind is in the Tera template for the top bar. Otherwise, we would need to convert the enum to and from a string every time we retrieved the data from the database. Please let me know if this is a good approach.Closes #2356