-
Notifications
You must be signed in to change notification settings - Fork 160
Replace hookdoc with WP Hooks Documentor #1337
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
base: develop
Are you sure you want to change the base?
Conversation
dkotter
left a comment
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.
Code looks good here but I agree on the note around the yellow color.
This is very hard to read and I expect does not pass color contrast accessibility. This color works on the Distributor site because we only use it when the background is black. But on a white background, you can barely read the yellow text.
There's a few approaches I could see here:
- Only have the dark mode theme and don't allow switching to light mode. That matches how our plugin site looks so may be fine
- Anywhere we have yellow text, ensure the background is dark. This would be similar to always forcing dark mode but would still retain some lighter elements
- (easiest option probably) Don't use that yellow color for text if the text is on a light background. Not sure what color to replace it with though, could just always use black but not as fun visually then
Open to other suggestions / opinions on this though, cc @jeffpaul
|
I deployed this on my fork repo here: https://iamdharmesh.github.io/distributor/ to get feedback on color in light mode. cc: @jeffpaul |
|
@iamdharmesh One other thing we'll need to take care of here that I just realized we missed in ClassifAI is to update any existing links within our code that points to the old documentation site. I know there's some in the changelog and looks like some in the documentation itself |
peterwilsoncc
left a comment
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, in addition to the accessibility issues I've got a couple of notes:
- defined in lines in the source section, eg on this page:
- would it be possible to link to the permalink rather than
trunk, eghttps://github.com/10up/distributor/blob/[commit-hash]/includes/utils.php#L318 - would it be possible to highlight the start of the docblock through to the
apply_filters|do_actions, eghttps://github.com/10up/distributor/blob/4831cdcc01ac23f32f28b5bb51c683a610d7c34c/includes/utils.php#L311-L322
- would it be possible to link to the permalink rather than
| <a href="https://github.com/10up/distributor/issues/"> | ||
| Support | ||
| </a> |
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 kind of reads as a donation request as is.
| <a href="https://github.com/10up/distributor/issues/"> | |
| Support | |
| </a> | |
| <a href="https://github.com/10up/distributor/issues/"> | |
| Issues | |
| </a> |
| target="_blank" | ||
| rel="noopener nofollow noreferrer" |
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.
target=_blank is really bad for accessibility, let's give users the choice.
| target="_blank" | |
| rel="noopener nofollow noreferrer" |
🔢 Applies to the other social links too.
| title="10up on Facebook" | ||
| aria-hidden="true" |
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.
At the moment these don't appear in the accessibility tree (checked with firefox dev tools)
| title="10up on Facebook" | |
| aria-hidden="true" | |
| aria-label="10up on Facebook" |
🔢 Applies to the other social links too
|
@iamdharmesh thanks for the PR! Could you please rebase your PR on top of the latest changes in the base branch? |
Description of the Change
PR replace the existing hookdoc package with the WP Hooks Documentor package which is used for generate developer docs. The new package is uses the docusaurus and provide good flexibility on extending docs site.
Note
The Distributor's yellow color is a bit light for the light theme, so I used a slightly darker shade here. Please let me know if we should use a different color instead. Dark mode looks fine, no changes there.
Closes #1336
How to test the Change
npm i && npm run build:docscd ./docs-build && npm run serveChangelog Entry
Credits
Props @jeffpaul, @iamdharmesh
Checklist: