-
Notifications
You must be signed in to change notification settings - Fork 239
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
feat: show issue/PR author-role indicators in UI, after author logins #516
base: main
Are you sure you want to change the base?
Conversation
8500d12
to
bae6862
Compare
1d3280a
to
be30fb6
Compare
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.
This is really great! Thanks for the contrib! 🚀
forgot to mention, can you also add this to the docs website? Should be easy to find where to add it |
be30fb6
to
05765b5
Compare
I’ve pushed the other changes I made in response to review comments — but I’ve not yet made the updates to the docs site. I’ll make time to do that after ~12 hours or so from now. |
05765b5
to
175aa1a
Compare
175aa1a
to
3efc99b
Compare
OK — I just now pushed an update (amended commit) with the following changes:
https://github.com/dlvhdr/gh-dash/compare/175aa1a506ae7fa69b3b6a1dac301312409a3353..3efc99b7d0e55117b6687e053831a79ca022befd has the delta diff against what you’d previously reviewed. |
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.
Thanks for working on this!
4131056
to
0501cfd
Compare
OK, thanks — I’ve just pushed an update (amended commit) that includes changes in response to all pending comments.
Thanks — those look great. However, I feel a bit that users might potentially be confused somewhat by what the Anyway, what I have for now is this: Is that the correct mappings/order for what you had in mind? If not, please lemme know what I should change. |
0501cfd
to
e1b03ca
Compare
https://github.com/dlvhdr/gh-dash/compare/3efc99b7d0e55117b6687e053831a79ca022befd..e1b03ca6ab6bf1772bf930b79f72f20a856a1616?w=1 is the delta against what you had previously reviewed |
Summary
This change causes author-role indicators (colored Nerd Font icons) to be shown the dashboard, after the author’s GitHub login. Each indicator shows the author’s role relative to the repo to which they submitted their PR or issue. Fixes #515.
For example, for a PR that’s the first PR the author has contributed to a particular repo, an indicator that the author is a first-time contributor to that repo will be shown.
Other indicators are used for showing that the author has previously contributed to the repo for the PR or issue, or that the author is member or owner of that repo, or a collaborator for that repo.
See https://docs.github.com/en/graphql/reference/enums#commentauthorassociation for details about the possible role (“author association”) values, or see the writeup at https://michaelheap.com/github-actions-check-permission/.
Note
The screenshots below show how the author-role-indicator Nerd Font icons would look in the UI after this change.
I don’t feel strongly at all about gh-dash needing to use the particular set of Nerd Font icons I chose in the current patch in this PR branch. So, @dlvhdr: If you have a different set of icons that you’d prefer — to best match with the rest of the existing gh-dash UI design — (or other commenters here have some good alternative suggestions), then I’m 100% fine with some other alternatives might be decided on instead.
How did you test this change?
I ran
make test
(go test -v ./...
), to ensure this change didn’t regress any existing tests.Otherwise, I just tested this change manually by building this PR branch and then switching my config between the compact UI view and the non-compact UI view, and visually inspecting the displayed data to confirm that the author-role indicators are displayed as expected.
Images/Videos
The above two screenshots are using the following set of icons:
The above screenshot is using the following alternative set of icons:
But as I alluded to in the note at the top of the PR description: I don’t feel strongly about either of those sets of icons. Whatever other set might get decided on instead of either of those two would also be fine with me.