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

more sidebar customization #1488

Merged
merged 41 commits into from
Feb 11, 2021
Merged

more sidebar customization #1488

merged 41 commits into from
Feb 11, 2021

Conversation

maelle
Copy link
Collaborator

@maelle maelle commented Feb 8, 2021

Fix #1443

  • Let users change the order of sidebar elements
  • Let users add custom sidebar sections (title, text that has to be HTML)
  • Let users completely suppress the navbar (even "Dev status")
  • Let users provide their own HTML for the navbar. I am not happy with this filepath solution because it is confusing compared to using templates. 🤔

TODO

  • Docs
  • Tests
  • ...

@maelle
Copy link
Collaborator Author

maelle commented Feb 8, 2021

Regarding the question in the issue about images, I suppose "text" could contain an img tag. 🤔

R/build-home-index.R Outdated Show resolved Hide resolved
R/build-home.R Outdated Show resolved Hide resolved
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

It's also useful to write the NEW.md bullet because that gives me the big picture view of goal of the PR.

R/build-home.R Outdated Show resolved Hide resolved
R/build-home-index.R Outdated Show resolved Hide resolved
return(pkg$meta$home$sidebar)

sidebar_path <- file.path(pkg$src_path, pkg$meta$home$sidebar$html)
Copy link
Member

Choose a reason for hiding this comment

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

If this is a pointer to a path, it should be html_path or similar. I wonder if this should be in addition to the regular sidebar, rather than a replacement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure.

  • If you want a really custom sidebar you'd provide your own HTML as sidebar.html field.
  • If you want to add a custom section (H2 title, any HTML below that), you'd use the sidebar.structure and sidebar.components fields.

Copy link
Member

Choose a reason for hiding this comment

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

Do we have evidence that folks want to replace the whole sidebar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no but this PR already provides a way to add custom component to the navbar.

R/build-home-index.R Outdated Show resolved Hide resolved
R/build-home-index.R Outdated Show resolved Hide resolved
R/build-home.R Outdated Show resolved Hide resolved
R/build-home.R Outdated Show resolved Hide resolved
R/build-home.R Outdated Show resolved Hide resolved
R/html-tweak.R Outdated Show resolved Hide resolved
R/utils.r Outdated Show resolved Hide resolved
R/build-home-index.R Outdated Show resolved Hide resolved
R/utils.r Show resolved Hide resolved
@@ -74,15 +74,19 @@ read_desc <- function(path = ".") {

# Metadata ----------------------------------------------------------------

read_meta <- function(path) {
path <- path_first_existing(
pkgdown_config_path <- function(path) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not in scope for this PR but with such a function one could easily have a helper file.edit(pkgdown_config_path("."))

@maelle maelle marked this pull request as ready for review February 9, 2021 13:54
R/build-home-index.R Outdated Show resolved Hide resolved
R/build-home-index.R Outdated Show resolved Hide resolved
R/build-home-index.R Outdated Show resolved Hide resolved
R/build-home.R Outdated Show resolved Hide resolved
R/build-home.R Outdated Show resolved Hide resolved
tests/testthat/test-data_home_sidebar.R Outdated Show resolved Hide resolved
tests/testthat/test-data_home_sidebar.R Outdated Show resolved Hide resolved
tests/testthat/test-data_home_sidebar.R Show resolved Hide resolved
tests/testthat/test-data_home_sidebar.R Outdated Show resolved Hide resolved
tests/testthat/test-data_home_sidebar.R Outdated Show resolved Hide resolved
maelle and others added 7 commits February 9, 2021 15:05
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
@maelle
Copy link
Collaborator Author

maelle commented Feb 11, 2021

The failures on macOS are due to the installation of textshaping, that has been released on CRAN yesterday.

@maelle maelle requested a review from hadley February 11, 2021 14:16
@maelle
Copy link
Collaborator Author

maelle commented Feb 11, 2021

I had missed the markdown_text() function, maybe that'd be a better interface for the custom sidebar sections i.e. calling the field markdown, not html. 🤔

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

I like the idea of using markdown instead of html (especially since you can still use raw html in markdown). But lets finish up the style issues in this PR, merge it, and then use markdown instead of html.

R/build-home-index.R Outdated Show resolved Hide resolved
R/build-home-index.R Outdated Show resolved Hide resolved
R/build-home-index.R Show resolved Hide resolved
R/build-home.R Outdated Show resolved Hide resolved
R/utils.r Outdated Show resolved Hide resolved
R/html-tweak.R Outdated Show resolved Hide resolved
R/html-tweak.R Show resolved Hide resolved
R/html-tweak.R Outdated Show resolved Hide resolved
R/html-tweak.R Outdated Show resolved Hide resolved
R/utils.r Show resolved Hide resolved
maelle and others added 4 commits February 11, 2021 15:50
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
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.

Feature request: Additional sidebar entries
2 participants