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

Allow footer links #114

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Allow footer links #114

wants to merge 4 commits into from

Conversation

cjrace
Copy link
Collaborator

@cjrace cjrace commented Dec 6, 2024

Overview of changes

Resolves #77, by adding an extra argument to the footer, allowing a vector of link text to be added, automatically generating actionLinks and IDs that can be used to control a hidden tabset.

Gone for this approach as I think it balances simplicity for the users with the functional need. As R Shiny is almost always a single page app, I assume most people would be using actionLinks to trigger a tabset shifting rather than actual href's to external pages. If we think this is a need, then I'll need to think again about the argument users can use. An alternative could be to allow users to put in any shiny tagList they deem appropriate, would be easier for us but then opens the door to more weird and wonderful stuff that might not be ideal or have the right styling.

Have added a functional example to the run_example() app too.

image

PR Checklist

  • I have updated the documentation (if needed)
  • I have added or updated tests for these changes (if applicable)
  • I have tested these changes locally using devtools::check()

Reviewer instructions

  1. Does it work as you'd expect?
  2. Is the documentation intuitive to follow?
  3. Is there enough test coverage?
  4. Should we be allowing standard anchor links in this as well? (if so, then probably best to do that now before merging)
  5. Do we still need the full = TRUE / FALSE option? Having poke at it a bit, I can see difference in the output code, but can't work out the difference for an end user of a dashboard? If we don't, I could try to tidy that up in this PR too?
  6. Should the crown symbol be on the right? Just noticed in my screenshot it wraps underneath, which feels odd, that wasn't something I've consciously done!

@cjrace cjrace added the enhancement New feature or request label Dec 6, 2024
@cjrace cjrace requested a review from sarahmwong December 6, 2024 09:00
Copy link
Collaborator

@sarahmwong sarahmwong left a comment

Choose a reason for hiding this comment

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

Thanks Cam!

  1. Works as expected
  2. documentation straightforward and easy to follow (Example in the function doesnt have an accessibility page to click into, dont htink thats a problem though as shows the layout/the cookie link functions fine
  3. tests are sufficient
  4. happy to only include internal shiny app links like you suggest - dont see why there would be a need for external links but could be one to monitor if anyone raises
  5. Happy to get rid of full argument
  6. Yes think image should be on the right if possible! Happy to pick up if this is a faff and you are heading off for xmas soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to add links into the footer
2 participants