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

Expand features to include callouts, headings, lists, and more (per #1) #4

Merged
merged 52 commits into from
Dec 14, 2023

Conversation

elipousson
Copy link
Contributor

Per #1 this PR includes the following new functions:

  • qto_block() and qto_div()
  • qto_span()
  • qto_dl(), qto_li(), and qto_ol()
  • qto_heading() and qto_hr()
  • qto_fig()
  • qto_shortcode() (and related functions)

These functions are supported by a variety of helper functions that should make it easier to continue building out the features of this package.

Supported by `qto_attributes` helper function and new utilities in utilities.R and utilities-list.R
`qto_heading()` is required for  `qto_callout()`
Also `qto_video()` and `qto_pagebreak()` and `qto_kbd()`

Also adds `check_src()` (used by `qto_video()`)
Adds allow_null parameter to combine helper function (and related embrace + bracket + parentheses functions)
Also

- drop sep parameter from qto_div (it didn't do anything)
- expose sep parameter for qto_block
Also rename qto_definition_list to qto_dl
Must include a new line before list

Also expose sep parameter to switch between tight + wide list spacing + add example showing option for nested lists + expose symbol parameter for qto_ol
@ElianHugh ElianHugh self-requested a review December 11, 2023 02:32
@ElianHugh
Copy link
Owner

ElianHugh commented Dec 11, 2023

Thank you for the PR, this looks great! I've had a short pass over the build with a few comments, but happy to have a discussion on anything.

Before committing I'd like to do a final pass to drop some old functions, fix some lintr issues, and fix some RCMD check issues

Edit: whoops. I forgot to actually hit the submit review button :)

R/block.R Show resolved Hide resolved
R/block.R Outdated Show resolved Hide resolved
R/basics.R Show resolved Hide resolved
R/map.R Show resolved Hide resolved
R/figures.R Outdated Show resolved Hide resolved
R/attributes.R Outdated Show resolved Hide resolved
@elipousson
Copy link
Contributor Author

I think I resolved all of the issues you flagged in your review and filled in a lot of missing documentation so cleaning up the check messages should be a good bit faster. I left the feedback on qto_hr() and map_qto() unresolved just in case there you wanted further changes to the former or to open an issue for the latter before we call it done.

Hopefully, I didn't introduce any new issues with the documentation clean-up! The only item I haven't done is add more tests but I figured you may either have suggestions on how you want that done or you may want to add them yourself as part of that "final pass".

One suggestion for mdapply(): I wonder if reworking it as a pmap_qto() function might be a good approach – should work much the same and provide a parallel to map_qto().

- Add standalones to lintr ignore
- Use explicit integers
- Match lintr indentation
- Implicit return
- Fix description typo of class and id using the opposite identifier
@ElianHugh
Copy link
Owner

Hi Eli, thanks again for submitting the PR. I've just pushed some minor changes for style/linter appeasement, and fixed a few typos. I've also marked mdapply and as_markdown as internal, and will shortly follow up with another PR implementing:

  1. pmap_qto
  2. removal of div, span, mdapply, and as_markdown functions
  3. change the with_* functions to use qto_div instead of the superceded div function

If you're happy with how things are with the PR, I'll go ahead and squash merge it!

@elipousson
Copy link
Contributor Author

Looks good to me. Thanks!

@ElianHugh ElianHugh merged commit c102a7b into ElianHugh:main Dec 14, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants