Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat!: Introduce
navbar_options()
#1141feat!: Introduce
navbar_options()
#1141Changes from 29 commits
01ff848
9ed36bb
0612d30
c726c6b
284d10f
8abc2b7
388720b
33732ae
f24c0c6
43e01c6
15c42d4
85b282a
a5b2868
5e1f0e2
b017dc5
7d473ee
fcd6d5c
3be0434
bd43c4d
a85ff8a
0fc9985
bf0f402
6f66bb7
76a9692
60b84ce
3f0d9bd
7028fba
bc87599
cf8d423
ee683e7
b919ae6
963a1eb
b163ffa
01a273c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think this is here to account for the fact that we might in the future have deprecated options that aren't available in
navbar_options()
? It's not clear to me why we need to account for that situation here, but then silently drop them later inrlang::exec(navbar_options, !!!options_user)
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.
Are you referring specifically to the
!opt %in% names(options_user)
line?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.
yep
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.
No, it says that
NULL
values innavbar_options()
can be overridden by non-null direct values, but I've changed this in the latest commit.I've reworked this so that
navbar_options(bg = NULL)
wins in the presence of a direct use ofbg
, since the signal is that the user putbg
innavbar_options()
. The implicitbg = NULL
innavbar_options()
is (still) overridden by a directbg = "red"
or similar.The end result is that
navbar_options_resolve_deprecated()
returns an object that is completely equivalent to one that would be created by directly callingnavbar_options()
. This means that when we elevate to deprecation errors instead of warnings, the only code we need to delete is the unreachable branch insidenavbar_options_resolve_deprecated()
and some easy-to-identify test code.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 would really really like to decouple
shiny::navbarPage()
frombslib::page_navbar()
. I'm thinking it'd be an update to addpage_navbar_legacy()
that we'd export but mark as internal so it doesn't show up in docs and indices.What I want out of that change is to have a clear line around what parts of the navbar API we shouldn't touch without thinking carefully about backwards compatibility or API changes and which parts are owned by bslib. Currently it feels too entangled and over time entropy wins leading to more entanglement.