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

Reactable example #107

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

Reactable example #107

wants to merge 4 commits into from

Conversation

alex-rogers-hub
Copy link

Pull request overview

Reactable example beneath datatable example
...

Pull request checklist

Please check if your PR fulfills 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())

What is the current behaviour?

...

What is the new behaviour?

...

Anything else

issue #84
...

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.

Couple of small comments to just remove data table. We're just wanting to push a single preferred solution.

Looks like a lot of lintr comments around unnecessary white space. Worth running styler::style_dir() if you haven't already.

Just to add as well, it'd be good to get some of the GDS style highlighting in as demoed in the css on one of our dashboards here:
https://github.com/dfe-analytical-services/local-authority-interactive-tool/blob/9f7aa4d06969614f55b002b50502a514d3c1d320/www/dfe_shiny_gov_style.css#L294

@@ -120,7 +120,11 @@ example_tab_1_panel <- function() {
)
),
# Benchmarking table --------------------------------------
dataTableOutput("tabBenchmark")
h2('An example Datatable'),
dataTableOutput("tabBenchmark"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think let's just remove the old reference to data table. We're moving towards reactable as we find it easier for to customise.

server.R Outdated
@@ -206,7 +206,7 @@ server <- function(input, output, session) {
height_svg = 5.0
)
})

# Benchmarking table
output$tabBenchmark <- renderDataTable({
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, let's just delete this, the idea is just to replace it with reactable

Copy link
Author

Choose a reason for hiding this comment

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

on it

paging = FALSE,
searching = FALSE
# defaultPageSize and minRows needed for Pagination: example here
# defaultPageSize = 4,

Check notice

Code scanning / lintr

Commented code should be removed. Note

Commented code should be removed.
searching = FALSE
# defaultPageSize and minRows needed for Pagination: example here
# defaultPageSize = 4,
# minRows = 4,

Check notice

Code scanning / lintr

Commented code should be removed. Note

Commented code should be removed.
@rmbielby rmbielby linked an issue Dec 19, 2024 that may be closed by this pull request
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.

Almost there I think! Just want to get an example of some css styling on the hover/focus etc in there that we can build on when we come to porting this into dfeshiny.

scrollX = TRUE,
paging = FALSE,
searching = FALSE
# defaultPageSize and minRows needed for Pagination: example here
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's avoid the commented out code. Just have it active (with some relatively appropriate values) and analysts can remove it if needed.

I'd also like to have highlight activated similarly to this table here:
https://github.com/dfe-analytical-services/local-authority-interactive-tool/blob/9f7aa4d06969614f55b002b50502a514d3c1d320/R/fn_table_helpers.R#L227

And then let's set an initial styling so we've got a starting point for editing the css as and when we come to that as well, e.g.:

headerClass = "bar-sort-header",

word-break: keep-all; /* Prevents breaking words mid-word */
white-space: normal; /* Allows line breaks between words */
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Initial css to back up the change requested in the reactable in server.R

Suggested change
.bar-sort-header:hover,
.bar-sort-header:focus {
background: #fd0;
}
/* Add a top bar on ascending sort */
.bar-sort-header[aria-sort="ascending"] {
box-shadow: inset 0 0.188rem 0 0 #1d70b8;
}
/* Add a bottom bar on descending sort */
.bar-sort-header[aria-sort="descending"] {
box-shadow: inset 0 -0.188rem 0 0 #1d70b8;
}
/* Add an animation when toggling between ascending and descending sort */
.bar-sort-header {
transition: box-shadow 0.3s cubic-bezier(0.175, 0.885, 0.32, 1.275);
}
/* Table row highlighting -------------------------------------------------- */
.rt-tr-highlight:hover {
background-color: #fd0;
}

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.

Show an example of using reactable with clear sort options
2 participants