Skip to content

RS-19683: Undo change to turn off flatten in 2d tables#83

Merged
chschan merged 2 commits intomasterfrom
RS-18683-undo-unflatten
Jun 4, 2025
Merged

RS-19683: Undo change to turn off flatten in 2d tables#83
chschan merged 2 commits intomasterfrom
RS-18683-undo-unflatten

Conversation

@chschan
Copy link
Contributor

@chschan chschan commented Jun 4, 2025

Undo change that may cause users existing visualizations to change. This means that spans in 2d tables will behave differently from 1d tables but better than having sudden changes occurring.

(This doesn't affect the fix for PPT exporting which happened with a 1d table - in that case, the span attribute was not dropped but was not subscripted appropriately. When flattened the whole span attribute is lost so it will not cause any problems for exporting/rendering).

@chschan chschan requested a review from JustinCCYap June 4, 2025 13:37
x <- ExtractChartData(x)
n.dim <- length(dim(x)) - isQTableWithMultStatistic(x)
if (n.dim > 2)
if (n.dim >= 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean to flatten a 2D table? To me it seems a 2D table doesn't need flattening?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I thought too. But it turns out the flattening is also removing the spans and adding it to the row/column labels: https://displayr.slack.com/archives/C013HGKCK9V/p1748868145503909

Copy link
Contributor

Choose a reason for hiding this comment

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

So with 1D tables we remove the spans and add it to the row labels?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to keep the spans for 1D tables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but it didn't do that before, so we don't want to change any existing behaviour (e.g, the 1-d chart in the original bug report would change if we tried to make this consistent).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think when we change to tidy tables, we should deal with this consistently, but for now I think we just try not to break anything that has already been created.

"I tend watch what I eat and drink, but don’t consider myself",
"I typically eat and drink whatever I feel like", "Male")), names = c("",
""), row.names = c(1L, 2L, 3L, 5L), class = "data.frame")))
# The following tests relate to spans in 2d tables
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect to restore this in the future? If not we shouldn't keep this.

@chschan chschan merged commit 54e3c0a into master Jun 4, 2025
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants