Skip to content

Strip and plot title text size#81

Merged
tdhock merged 8 commits intomasterfrom
strip-size
Mar 12, 2023
Merged

Strip and plot title text size#81
tdhock merged 8 commits intomasterfrom
strip-size

Conversation

@tdhock
Copy link
Copy Markdown
Collaborator

@tdhock tdhock commented Feb 2, 2023

this is a follow-up to #66 and #78
I implemented custom text sizes that can be specified in R code, for plot and panel/strip titles.
Since this is very similar to your GSOC project last year @Faye-yufan can you please review?

@tdhock
Copy link
Copy Markdown
Collaborator Author

tdhock commented Feb 2, 2023

also do you have any idea why the test is failing? It seems that we should be able to get the text size via text-size style, just like the axis ticks etc.

@Faye-yufan
Copy link
Copy Markdown
Contributor

Hi @tdhock , thanks for the follow-up!
For the test, it's probably because the font-size for the title is defined as a property instead of a style, and the test helper function getStyleValue() is for getting style only.
I assume we don't have any helper function to get the value of properties, so I created a new function called getPropertyValue(). It shall pass now.

Copy link
Copy Markdown
Contributor

@Faye-yufan Faye-yufan left a comment

Choose a reason for hiding this comment

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

It looks good to me!

Comment thread R/theme-defaults.r

strip.background = element_rect(fill = "grey85", colour = NA),
strip.text = element_text(colour = "grey10", size = rel(0.8)),
strip.text = element_text(colour = "grey10"),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So we will remove the rel size in animint2 aesthetic?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your review @Faye-yufan and for the fix in the tests.
I'm not sure I understand the question. I think rel is ok to use still, but this deletion of strip.text property was intended to increase the text size of the strip text, does that make sense?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah sure! I get it now.

@tdhock tdhock merged commit 5bb9d51 into master Mar 12, 2023
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.

2 participants