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

Bug fix: show = FALSE as default #105

Merged
merged 5 commits into from
Dec 14, 2020
Merged

Bug fix: show = FALSE as default #105

merged 5 commits into from
Dec 14, 2020

Conversation

csgillespie
Copy link
Contributor

@csgillespie csgillespie commented Dec 10, 2020

In the current version, this

reactable(iris, defaultColDef = colDef(show = FALSE), 
          columns = list(Species = colDef(show = TRUE)))

doesn't work.


My PR changes the line:

if (!show) FALSE

This meant that setting show = TRUE was treated as a NULL and stripped out. I simply changed that line to

show = show

to match the above style. The following examples work as expected:

reactable(iris, defaultColDef = colDef(show = FALSE), 
          columns = list(Species = colDef(show = TRUE)))

reactable(iris, 
          defaultColDef = colDef(show = TRUE), 
          columns = list(Species = colDef(show = FALSE)))

Copy link
Owner

@glin glin left a comment

Choose a reason for hiding this comment

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

Nice catch, thanks! I added some minor suggestions to bump to a dev version, and to add proper credit in NEWS.

I'd also like to handle the case where there's a default show = FALSE value, and a colDef() is provided without show. With the current changes, the default value is always overridden if colDef() has been provided. For example, the Sepal.Width column will be visible here:

reactable(
  iris,
  defaultColDef = colDef(show = FALSE),
  columns = list(
    Species = colDef(show = TRUE),
    Sepal.Width = colDef(name = "sepal width")  # Has show = TRUE by default
  )
)

I think a simple way to handle this would be to change the default value of show to NULL in colDef().

DESCRIPTION Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
Co-authored-by: Greg Lin <[email protected]>
@codecov-io
Copy link

codecov-io commented Dec 13, 2020

Codecov Report

Merging #105 (9eb6c61) into master (33255ab) will decrease coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
- Coverage   99.17%   99.02%   -0.16%     
==========================================
  Files          15        9       -6     
  Lines        1823     1125     -698     
  Branches      335      335              
==========================================
- Hits         1808     1114     -694     
+ Misses         15       11       -4     
Impacted Files Coverage Δ
R/language.R
R/utils.R
R/reactable.R
R/theme.R
R/shiny.R

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33255ab...9eb6c61. Read the comment docs.

@csgillespie
Copy link
Contributor Author

I've updated the default to NULL and tweaked the docs to match previous entries.

Copy link
Owner

@glin glin left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

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.

3 participants