Skip to content

Add spanner id as an argument to tab_spanner()#665

Merged
rich-iannone merged 36 commits intomasterfrom
spanner-id
Nov 9, 2020
Merged

Add spanner id as an argument to tab_spanner()#665
rich-iannone merged 36 commits intomasterfrom
spanner-id

Conversation

@rich-iannone
Copy link
Member

@rich-iannone rich-iannone commented Oct 23, 2020

This PR allows for the setting of a spanner ID value, which makes it easier to access a spanner column label through cells_column_spanners() (when using tab_style() or tab_footnote()]), it occurs through the idvalue (and not thelabel). If an id` is not provided here, it will be generated from the label, stripping any tags and special characters, and limiting the generated string to 10 characters.

Although a message will be provided in this case, it is advised that an explicit id value be set if planning to access this cell in a later function call. Also, when providing an id, it must be unique across all ID values set for column spanner labels.

Fixes: #521
Fixes: #626
Fixes: #597

@jcheng5 jcheng5 marked this pull request as ready for review October 23, 2020 19:44
@jcheng5
Copy link
Member

jcheng5 commented Oct 23, 2020

After discussing in person:

  1. No restriction on id; just use the label as-is by default.
  2. Don't attempt to automatically fix duplicate id's in tab_spanner; just stop with an error message informative enough that it's clear how they should proceed.
  3. For tab_spanner_delim, automatically fix duplicate id's using TBD method (tibble::name_repair?) and if that was necessary then let user know what the resulting id's are. Maybe we should have a param that lets them turn the message off.
  4. If we allow n-level spanner hierarchies I'd like to revisit this, I'd imagine uniqueness only mattering within a single scope (like filesystem hierarchies).

@jcheng5 jcheng5 marked this pull request as draft October 23, 2020 20:17
@rich-iannone rich-iannone marked this pull request as ready for review October 27, 2020 20:59
@rich-iannone rich-iannone requested a review from jcheng5 October 27, 2020 20:59
Copy link
Member

@jcheng5 jcheng5 left a comment

Choose a reason for hiding this comment

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

Just the nits from the comments

@rich-iannone rich-iannone changed the title Add spanner ID as optional argument to tab_spanner() Add spanner id as an argument to tab_spanner() Oct 29, 2020
Copy link
Member

@jcheng5 jcheng5 left a comment

Choose a reason for hiding this comment

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

  1. RTF needs to use spanner id's instead of labels.
  2. rle() needs to use spanner_id instead of spanner.
  3. Test case with adjacent spanners that use same label but different IDs; they should not be merged into one spanner.

@rich-iannone
Copy link
Member Author

  • RTF needs to use spanner id's instead of labels.
  • rle() needs to use spanner_id instead of spanner.
  • Test case with adjacent spanners that use same label but different IDs; they should not be merged into one spanner.

All of these items are addressed now and I took the time to fine tune the docs for tab_spanner() and tab_spanner_delim() as well.

@rich-iannone rich-iannone requested a review from jcheng5 November 3, 2020 03:53
Copy link
Member

@jcheng5 jcheng5 left a comment

Choose a reason for hiding this comment

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

  1. Test cases with spanner id != spanner label, for HTML, latex and rtf
  2. More test cases for latex and rtf

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