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

Part 3 - remove tab dependencies #43

Merged
merged 9 commits into from
Aug 29, 2024

Conversation

cjrace
Copy link
Contributor

@cjrace cjrace commented Jul 31, 2024

Worth reviewing #42 first, before then re-pointing this at the branch for #41 which we're using as a big branch of changes for v0.3.0

This changes the support_panel() and cookies_panel_ui() functions to only return the content and not specify the type of tab that it belongs too as mentioned in #41. Aim here is to give flexibility in how the content is used and not tie ourselves down too much. For ourselves we'll want to be switching to bslib tabs soon anyway, and also this allows users to add more content or customise their dashboards a little more, while preserving the core of what we want.

It will be a breaking change but better to do that while adoption is low now!

This also updates some of the testing around these functions to make it a bit more thorough / robust. The original shinytest2 snapshots for the support panel were only actually tracking the navlist input had changed rather than anything about the panel itself. As a result, I have:

  • Removed screenshots (they didn't seem necessary?)
  • Removed the shinytest2 ui tests on the test dashboard for the support panel
  • Left the code in the test dashboard for the support panel as it works as a nice minimal example app for the package and also allows us to check it doesn't break the app in anyway through the basic load test
  • Added unit tests that check the class and content of the support_panel() and cookies_panel_ui() functions

EDIT:
The test_dashboard tests were failing on the GHA servers but passing locally, brute forced some extra waits in and it seemed to do the trick.

@cjrace cjrace changed the base branch from main to add-branch-install-readme July 31, 2024 14:31
@cjrace cjrace changed the base branch from add-branch-install-readme to update-cookie-to-cookies July 31, 2024 14:32
@cjrace
Copy link
Contributor Author

cjrace commented Jul 31, 2024

@cjrace cjrace changed the title Part 2 - remove tab dependencies Part 3 - remove tab dependencies Aug 2, 2024
@cjrace
Copy link
Contributor Author

cjrace commented Aug 2, 2024

Base automatically changed from update-cookie-to-cookies to add-branch-install-readme August 7, 2024 14:21
cjrace added 2 commits August 19, 2024 11:17
Merge branch 'add-branch-install-readme' into remove-tab-dependencies

# Conflicts:
#	R/cookies.R
#	man/cookies.Rd
#	man/cookies_banner_server.Rd
#	man/cookies_banner_ui.Rd
#	man/cookies_panel_server.Rd
#	man/cookies_panel_ui.Rd
#	tests/test_dashboard/tests/testthat/_snaps/UI-01-basic_load/basic_load-001_.new.png
#	tests/test_dashboard/tests/testthat/_snaps/UI-02-cookies/cookies_consent-001_.png
#	tests/test_dashboard/tests/testthat/_snaps/UI-02-cookies/cookies_consent-002_.png
#	tests/test_dashboard/tests/testthat/_snaps/UI-02-cookies/cookies_consent-003_.png
#	tests/test_dashboard/tests/testthat/_snaps/UI-02-cookies/cookies_consent-004_.png
#	tests/test_dashboard/tests/testthat/_snaps/UI-03-support_panel/support_panel-001.json
@cjrace cjrace requested a review from rmbielby August 19, 2024 10:28
@cjrace cjrace marked this pull request as ready for review August 19, 2024 10:28
@cjrace cjrace merged commit 8acf6aa into add-branch-install-readme Aug 29, 2024
6 checks passed
@cjrace cjrace deleted the remove-tab-dependencies branch August 29, 2024 14:38
cjrace added a commit that referenced this pull request Sep 4, 2024
* Bump roxygen2 version to latest

* Add branch note to readme and copy text from dfeR, add contributing file

* add lintr config with limit matching CRAN and style_dir() to tidy code

* add PR template

* fix contributing headers

* tidy up after conflict resolution

* attempt a github action for the test dashboard

* try to stop false positives coming from global variable binding by pre-loading the packages

* remove renv from test dashboard yaml

* fix typo in test_dashboard gha

* try adding devtools to description (test_dashboard gha)

* make sure that GHA will fail if the test dashboard tests fail

* Revert "make sure that GHA will fail if the test dashboard tests fail"

This reverts commit 1503c49.

* put the local package into the setup r dependencies step, update CodeQL version

* add a code of conduct (same as dfeR)

* copy dfeR issue templates

* update test_dashboard tests (did some tidy up while looking at adding something for the support panel)

* rebuild favicons to make them appear on pkgdown site

* linting and responding to test notes

* fix brackets in code of conduct

* add missing families and update ids in test dashboard

* Increment version number to 0.3.0

* PR response and standardise examples / vignette for cookies

* Add init_analytics() into news!

* remove duplicate branch install instructions (turns out we had added it on another branch!)

* run r cmd check on all pull requests

* Part 1 - update cookie to cookies (#42)

* update cookie to cookies

* run r cmd check on all pull requests

* Part 2 - Make init code inline (#44)

* Update init_analytics to have a create_file option and add more unit tests around the content

* Move the init_analytics HTML / JS to be written inline

* Prevent init_analytics from running in examples and creating scripts in the check directory

* update init_cookies to have the JS code inline and add some tests

* Part 3 - remove tab dependencies (#43)

* update cookie to cookies

* remove tabPanel from within our functions

* remove images from snapshots as not needed

* Switch support panel tests to unit tests (as the UI ones didn't track anything beyond if the tab was selected)

* Add tests for cookies_panel_ui

* Add note to contributing guidelines about lintr and loading package first

* run r cmd check on all PRs

* force some extra waits

* Clarified usethis makes blank scripts and have updated readme so users will install the correct package

* update news.md with changes on this branch

* update news.md
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