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

Behind the scenes tidy up #78

Merged
merged 57 commits into from
Aug 6, 2024
Merged

Behind the scenes tidy up #78

merged 57 commits into from
Aug 6, 2024

Conversation

cjrace
Copy link
Contributor

@cjrace cjrace commented Jul 19, 2024

Pull request overview

Doing a general tidy up of the template ahead of creating a new repo, aiming to resolve some known issues.

Considered updating to bslib though deciding that should be a separate PR as this was already big enough to review. Raised a number of issues in the Accessibility UI refresh milestone to cover those.

Pull request checklist

Please check if your PR fulfils the following:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Tests have been run locally and are passing (shinytest2::test_app())
  • Code is styled according to tidyverse styling (checked locally with styler::style_dir() and lintr::lint_dir())

Details of changes

Lots, sorry! Happy to have a call with people if easier.

Automated tests

CI

Code tidy up

  • Ran lintr::lintr_dir() locally and resolved all issues so there's a clean starting point
  • Updated references from stats development to EES team
  • Added README.html to .gitignore to avoid anyone accidentally committing a html version and it going out of date
  • Moved code around to reduce number of scripts we have, and to make a folder for separate UI panels to live in to keep the individual scripts shorter and easier to manage
  • Updated some of our comments around code, including the CSS file
  • Removed the custom CSS loading screen, could be convinced to bring back if we really want it
  • Updated the README to be focussed around a template that others can make use of and easily update

ggplot element_line()

  • The size argument is now depreciated so I've followed the warning message to update it
    image

Styling

  • Updated text CSS to use Arial consistently as our text was anything but consistent before
  • Updated accessibility statement to use p() tags instead of br() which was causing unusual spacing

Anything else

  • Simplified the code behind the value boxes to allow for exporting reactive values (though also I think makes it easier to read), and dropped some of the example value boxes as they were a bit overwhelming on the first page. This also gives the benefit of making it visually clear we've made an update

  • One of the reasons for showing the value exporting approach to UI tests is that I think we should be encouraging that moving forwards. shinytest2's documentation on suggested approaches certainly does:

It cannot be recommended enough to use shiny::exportTestValues() to test your Shiny app’s reactive values.

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

# testServer(expr = {
# # Give the inputs expected on initial load
# session$setInputs(
# navlistPanel = "dashboard",

Check notice

Code scanning / lintr

Commented code should be removed.

Commented code should be removed.
# # Give the inputs expected on initial load
# session$setInputs(
# navlistPanel = "dashboard",
# region = "England",

Check notice

Code scanning / lintr

Commented code should be removed.

Commented code should be removed.
# session$setInputs(
# navlistPanel = "dashboard",
# region = "England",
# selectPhase = "All local authority maintained schools",

Check notice

Code scanning / lintr

Commented code should be removed.

Commented code should be removed.
# navlistPanel = "dashboard",
# region = "England",
# selectPhase = "All local authority maintained schools",
# tabsetpanels = "Benchmark example",

Check notice

Code scanning / lintr

Commented code should be removed.

Commented code should be removed.
# region = "England",
# selectPhase = "All local authority maintained schools",
# tabsetpanels = "Benchmark example",
# selectBenchLAs = ""

Check notice

Code scanning / lintr

Commented code should be removed.

Commented code should be removed.
#
#
# # Check the reactive benchmark table is a valid number
# # expect_true(grepl("^\\d*$", reactive_benchmark()))

Check notice

Code scanning / lintr

Commented code should be removed.

Commented code should be removed.
# # Check the reactive benchmark table is a valid number
# # expect_true(grepl("^\\d*$", reactive_benchmark()))
#
# print(reactive_benchmark())

Check notice

Code scanning / lintr

Commented code should be removed.

Commented code should be removed.
# # expect_true(grepl("^\\d*$", reactive_benchmark()))
#
# print(reactive_benchmark())
# print(output$tabBenchmark)

Check notice

Code scanning / lintr

Commented code should be removed.

Commented code should be removed.
# print(reactive_benchmark())
# print(output$tabBenchmark)
#
# reac_benc_tab <<- output$tabBenchmark

Check notice

Code scanning / lintr

Commented code should be removed.

Commented code should be removed.
# reac_benc_tab <<- output$tabBenchmark
#
# # Check the output has expected number of rows for benchmark table
# expect_equal(nrow(reactive_benchmark()), 3)

Check notice

Code scanning / lintr

Commented code should be removed.

Commented code should be removed.
@cjrace cjrace changed the title General tidy up Behind the scenes tidy up Jul 22, 2024
@cjrace cjrace marked this pull request as ready for review July 22, 2024 07:43
@cjrace
Copy link
Contributor Author

cjrace commented Jul 22, 2024

@cjrace cjrace requested a review from rmbielby July 22, 2024 16:25
Copy link
Contributor

@rmbielby rmbielby left a comment

Choose a reason for hiding this comment

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

Might review this in chunks, here's some initial thoughts from a first sweep...

R/utils.R Outdated Show resolved Hide resolved
www/dfe_shiny_gov_style.css Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
tests/testthat/test-Server-01-example_dropdown_filtering.R Outdated Show resolved Hide resolved
tests/testthat/test-UI-01-basic_load.R Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@cjrace cjrace assigned cjrace and unassigned rmbielby Jul 24, 2024
Copy link
Contributor

@rmbielby rmbielby left a comment

Choose a reason for hiding this comment

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

A few minor suggestions

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README_template.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@cjrace cjrace assigned rmbielby and unassigned cjrace Aug 6, 2024
@cjrace cjrace merged commit 074cfde into main Aug 6, 2024
3 checks passed
@cjrace cjrace deleted the general-tidy-up branch August 6, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants