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
ARROW-17887: [R][Doc] Improve readability of the Get Started and README pages #14514
Conversation
Conflicts: r/vignettes/developers/setup.Rmd
…e, renames data objects vignette
Co-authored-by: Stephanie Hazlitt <7143861+stephhazlitt@users.noreply.github.com>
Co-authored-by: Stephanie Hazlitt <7143861+stephhazlitt@users.noreply.github.com>
Co-authored-by: Stephanie Hazlitt <7143861+stephhazlitt@users.noreply.github.com>
Co-authored-by: Stephanie Hazlitt <7143861+stephhazlitt@users.noreply.github.com>
Co-authored-by: Stephanie Hazlitt <7143861+stephhazlitt@users.noreply.github.com>
Co-authored-by: Stephanie Hazlitt <7143861+stephhazlitt@users.noreply.github.com>
okay so we might be ready to merge this? I'm running out of things that I want to try fixing in this iteration of improvements |
I've removed the WIP flag from the issue title to run the CI on it. I'm happy to give the contents a thumbs up and defer any more changes to follow-up tickets. Before I approve it, I'm going to pull it locally and build it there just to give it a final look over all together. @nealrichardson - did you want to give this a look over as well? |
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've built this locally and it looks good!
Thanks for your hard work on this @djnavarro! This is a significant contribution and makes a lot of things clearer for users.
One small note - it was my error as a maintainer to not insist on splitting up this PR into a series of smaller PRs; though I see why it made sense to do it all in one here given content is being moved between sections, in practice it's been a bit tricky to keep up with what has or hasn't been reviewed, and it also can make it tricky for anyone who wants to later look at the source of a change. Please can you split changes up into smaller PRs on future contributions?
The upgrade to Bootstrap 5 will break the version dropdown selector, but I've opened ARROW-18391 to fix it, and will look at that myself shortly.
One thing I noticed, which might be worth a follow up ticket (or maybe I am being too picky) -- in the |
I approved this before the CI had run, but there are a few failures relating to linting that need to be sorted before we merge. |
@thisisnic I think all the linting issues are now fixed. And yes, I agree this PR is too big to review easily: I'll work harder at splitting into separate PRs in future 🙂 |
@stephhazlitt Yeah I noticed that issue with the developer vignettes earlier. It would be nice if we could have a menu item that linked to that subsection of the articles page. That way devs would be able to click through with about the same level of ease as they have under the current docs where dev vignettes are tucked into a submenu. I haven't quite worked out how to do that with the bootstrap5 pkgdown templates yet. Tempted to push that to a separate PR 😁 |
+1 for sure for pushing that into a subsequent ticket/PR @djnavarro. Great, great work on this PR. |
Benchmark runs are scheduled for baseline = 409a95d and contender = 4afe710. 4afe710 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
This pull request proposes a number of changes to the pkgdown site for the R package:
In addition there are some structural changes:
Some vignette filenames have been edited and links updated(this has been reverted)The developer vignettes are now in the main vignettes folder(this has been reverted)Possible issues as yet unaddressed: