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

Writing custom check functions vignette #127

Merged
merged 48 commits into from
Oct 11, 2024

Conversation

annakrystalli
Copy link
Member

@annakrystalli annakrystalli commented Oct 4, 2024

This PR adds:

I've also pulled out information that is useful to both the writing and deploying custom functions vignette into children documents which are now deployed in both. Because of this it might be more useful to have the rendered vignette open to see the content in place.

Copy link

github-actions bot commented Oct 4, 2024

Copy link
Contributor

@LucieContamin LucieContamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much! It globally looks all good and make sense. I like the examples. I just made some minor comments.

vignettes/articles/children/_custom-fn-available-args.Rmd Outdated Show resolved Hide resolved
vignettes/articles/children/_custom-fn-available-args.Rmd Outdated Show resolved Hide resolved
vignettes/articles/deploying-custom-functions.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a monumental effort! Thank you for writing this, Anna!

One minor comment about formatting: Vignettes should start with heading level 2. The title takes the level 1 heading and pkgdown will automatically convert all the headings for you. This is an accessibility technique to create semantic structure: https://webaim.org/techniques/headings/

Otherwise, I have some suggestions and clarifications that I think will strengthen the vignette. I felt that the discussion in _custom-fn-available_args was a bit difficult to understand and I attempted to have a go at it (though I don't know how well I did).

vignettes/articles/validate-pr.Rmd Outdated Show resolved Hide resolved
vignettes/articles/validate-pr.Rmd Outdated Show resolved Hide resolved
vignettes/articles/validate-pr.Rmd Outdated Show resolved Hide resolved
vignettes/articles/validate-submission.Rmd Outdated Show resolved Hide resolved
vignettes/articles/children/_add-deps-source.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
@annakrystalli
Copy link
Member Author

Thanks both @LucieContamin & @zkamvar ! I've taken your super helpful suggestions and reworked the vignettes so hopefully things are clearer. Let me know your thoughts!

Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thank you for your diligence on this.

I have a couple of minor formatting changes that ensures the alert shows up, ensures the lists are formatted correctly, and that headings are headings.

vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/children/_custom-fn-available-args.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/children/_custom-fn-available-args.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/validate-pr.Rmd Show resolved Hide resolved
Copy link
Contributor

@LucieContamin LucieContamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks all good to me! thank you very much for all the work!

I just made some small suggestions, mostly about stuff not rendering properly on my side. If it's just on me, please ignore!

vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/writing-custom-fns.Rmd Outdated Show resolved Hide resolved
vignettes/articles/children/_custom-fn-available-args.Rmd Outdated Show resolved Hide resolved
vignettes/articles/children/_custom-fn-available-args.Rmd Outdated Show resolved Hide resolved
@annakrystalli
Copy link
Member Author

Thanks again both!

I incorporated the majority of your suggestions. Hopefully we're good to go now 😅

@annakrystalli annakrystalli requested a review from zkamvar October 11, 2024 08:30
Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good! The only thing left is to make sure the subheadings are actually marked as headings or combined with their parents.

I'm not trying to bike shed, I'm thinking about this from an accessibility standpoint. At the moment, they are only sub headings for people who use vision to read the document. For people who use screen readers, they are sentence fragments.

Here are a couple of resources that describe this:
WebAIM: Non-HTML headings (visual only)
WCAG 2.2: Failure of Success Criterion 1.3.1 due to using changes in text presentation to convey information without using the appropriate markup or text (note that the WCAG is the authority for internet accessibility).

If you don't think the subheadings add much to the structure, then I would suggest to combine them with the headings:

### Capturing and returning check results with `capture_check_cnd()`

### Skipping checks and returning a message with `capture_check_info()`

@annakrystalli
Copy link
Member Author

OK. I don't love such long headers and feel it visually burdens the page now but I've added them anyways to move this forward.

I did also remove the level 3 heading introduced in the warning box about tbl. Marking info as headers when they are not (it's really a box title) is also not good for accessibility. Most importantly it had no business in the TOC

image

@zkamvar zkamvar self-requested a review October 11, 2024 17:44
@annakrystalli annakrystalli merged commit 79d32a7 into main Oct 11, 2024
8 checks passed
@annakrystalli annakrystalli deleted the ak/custom-fn-dev-article/121 branch October 11, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
3 participants