-
Notifications
You must be signed in to change notification settings - Fork 314
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
Use js to show status of gh issues and PR #1896
base: main
Are you sure you want to change the base?
Conversation
Can we add @isabela-pf to the repo (even only with read access) so that we can assign her to review ? |
I wonder if the tool that mystmd.org uses for GitHub hover cards could be repurposed here? @agoose77 or @stevejpurves any thoughts on that? Either way I think that updating the statuses is pretty nice 🙂 |
@Carreau there was a long-ish discussion before on whether this was a good idea, that ended in a stalemate: #1085 FWIW my views (one, two) on this haven't really changed since then. But there's enough new voices now that I could be outvoted! EDIT: I realize now that the two proposals are different; here you just want to add the icons, not convert the link into the full PR title. I'm OK with adding/coloring the icons. |
invitation sent! |
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.
EDIT: I realize now that the two proposals are different; here you just want to add the icons, not convert the link into the full PR title. I'm OK with adding/coloring the icons.
I'm glad you called this out, because I was going to double check on the same after reading preceding issues. Thank you!
And many thanks to @Carreau for posting the screen shot of the result! That also makes it much easier for me to be sure what I'm looking at.
I'm going to write my comments as a numbered list for (hopefully) easier reference, but they are not reliant on one another.
- The icon choice is good! I am glad to see them used in the same way as the GitHub reference discussed previously.
- The addition of icons also makes sense considering GitHub as a reference. Do the icons get announced as a part of the link or in the tooltip, or are they visual only?
- I think it was a good idea to map the status colors in the GitHub reference to the ones within the theme, and I think appropriate colors were chosen. However, in this situation, I do worry that they are close in value (light/darkness) and could be easily mixed up due to colorblindness, low or bright light situations, and any other situation where color is harder to discern. If the status is reiterated in the tooltip or in a text precursor to the
#xxxx
I think this is acceptable. If color is the only way to tell this status, I'd like to advise an additional way of communicating.
Please let me know if you have any questions for me! This is definitely on a good track, self-appointed "less than optimal design skills" or not.
Thanks for the links to #1085, I missed it. I think this is indeed much narrower in scope.
Currently those are just visual, we can change if we want.
I think that's my main question, I'm really not good ad design/color. If you think there are changes we need to do and you can come up with colors that work with PST that would help me. I've mostly use red/green/purple because I know that's what GitHub is doing. I don't want to bias the design process, but should the link be colors the same way as the icon, and should for example closed issues be strikethrough. I'm tempted to think no for strikethrough as most issue reference in documentation will be to merged PR and closed issues. |
I likely need to use font-awesome 6 instead of 5. |
or even octicons? I think a complete set for us would be:
font awesome has code-merge and code-pull-request and circle-check but its circle-dot doesn't really resemble the octicon equivalent. |
I'm happy to use octicon, I just wanted to avoid an extra dependnecy by default. |
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'm weighing in one more time with more explicit approval on the design front in hopes that clarifies any lingering questions. In terms of what's been done so far, I find it matches GitHub's patterns of status icons and coloring, matches pydata-sphinx-theme's design system (uses color variables and relevant icons), and is mindful of dependencies that may mess with either of these patterns from a user experience/user interface perspective. That's all good to me!
I do think this is not a change that is needed to, for example, fix a bug or anything in the pydata-sphinx-theme, so if there is a choice to not move forward with this I also think that preserves a perfectly fine experience. But if it is wanted, this proof of concept seems like the right way to go about it from the design perspective.
We should be using FA 6 (this is the one we are vendoring with the theme). |
Also despite my earlier comment about octicons, I won't object to using FA6 equivalents --- we can always switch to octicons later if necessary. |
Sorry this was meant more at @Carreau since he is currently using FA 5 |
here's a screenshot of how this will look: IMO we're not there yet:
|
Nah, that is because the PR I linked to has been merged since then...
I did not figured out how to not underline it I'll have a look at the rest. |
See pydata#1895. Not that I heve the right skills and am good at desing, just show what could be done. Need to be pulled into a separate JS file and properely desinged.
fix trainling s not matching, and catching 403 due to exhausted requests.
<script type="module"> | ||
import { Octokit, App } from "https://esm.sh/octokit"; | ||
const octokit = new Octokit(); | ||
window.octokit = octokit; |
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.
Is there a good reason to make the octokit instance global?
window.octokit = octokit; |
See #1895.
Not that I heve the right skills and am good at desing, just show what could be done.
Need to be pulled into a separate JS file and properely desinged.