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

classNameDefinedColumns() now identifies negative and "_all" targets #512

Merged
merged 2 commits into from
Feb 25, 2018

Conversation

shrektan
Copy link
Collaborator

@shrektan shrektan commented Feb 25, 2018

closes #511

The fix #476 misses the fact that besides of column indexes, there're 4 kinds of columnDefs.targets in total.

  • 0 or a positive integer - column index counting from the left
  • A negative integer - column index counting from the right
  • A string - class name will be matched on the TH for the column (without a leading .) See the comment below.
  • The string "_all" - all columns (i.e. assign a default)

This PR is meant to handle all the rest cases. However, for case 3, I can't reproduce the effect in the first place. What I mean is explained by the following example:

UPDATE 20180425 This is because DT sends out the data as an array instead of an object. So the datatables library itself doesn't know the column names of each column.

DT::datatable(
  data.frame(a = 1, b = 2, c = 3, d = 4),
  options = list(
    columnDefs = list(
      list(className = "dt-center", targets = "a") 
      # it should have set "a" column center alignment, but it didn't
    )
  )
)

For 1, 2 and 4 cases, they are correctly handled now.

DT::datatable(
  data.frame(a = 1, b = 2, c = 3, d = 4),
  options = list(
    columnDefs = list(
      list(className = "dt-center", targets = "_all")
    )
  )
)

DT::datatable(
  data.frame(a = 1, b = 2, c = 3, d = 4),
  options = list(
    columnDefs = list(
      list(className = "dt-center", targets = c(-1, 2))
    )
  )
)

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Thanks!

@yihui yihui added this to the v0.5 milestone Feb 25, 2018
Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Can you also add an item in NEWS.md?

@shrektan
Copy link
Collaborator Author

shrektan commented Feb 25, 2018

My bad, added.

For the case 3 A string - class name will be matched on the TH for the column (without a leading .), any ideas why it won't work?

@yihui
Copy link
Member

yihui commented Feb 25, 2018

I have no idea. As long as #511 is fixed, I don't really care much for now. Thanks!

@yihui yihui merged commit b807c29 into rstudio:master Feb 25, 2018
yihui added a commit that referenced this pull request Feb 25, 2018
@vnijs
Copy link
Contributor

vnijs commented Feb 25, 2018

FYI Although the examples @shrektan created for these cases seem to work, I can still recreate issue if the first column is an integer using version 0.4.2 from GitHub (see screenshot and example below).

image

DT::datatable(
  sapply(iris[,1:3], as.integer),
  options = list(
    columnDefs = list(
      list(className = "dt-center", targets = "_all")
    )
  )
)

In the above example, the issue goes away if the data is in a data.frame rather than a matrix, but I can also reproduce the issue in a shiny app where the input is a data.frame (see screenshot below)

image

@shrektan
Copy link
Collaborator Author

shrektan commented Feb 26, 2018

@vnijs My bad. It must be something related to the row names... Will file a patch now... Apologize for the trouble.

@vnijs
Copy link
Contributor

vnijs commented Feb 26, 2018

No trouble all @shrektan. I'm grateful for your time and effort

@shrektan
Copy link
Collaborator Author

shrektan commented Feb 26, 2018

@vnijs Thanks. I've submitted the PR #514 to fix this. Let me know if still problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants