-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update theme template #14583
Update theme template #14583
Conversation
Thanks for the PR -- could you also share a screenshot of how the preview looks with your changes? Can you also confirm the entire file contents are visible with a font size of 12? (I think we have a bit of room to expand the height of the preview if necessary) |
I don't really have a dev environment, I just edited the file as you added it in #13560. There seems to be 4-5 empty lines at the end. As for the width, this line is the widest i <- c(n5, n4, n3, n2, n1) The added lines do not exceed that width. I could include a screenshot with the impact of the changes when the next daily is available? |
@@ -3,9 +3,11 @@ fivenum <- function(x) { | |||
|
|||
# handle empty input | |||
n <- length(x) | |||
if (n == 0) | |||
if (n == 0) { | |||
message("No input") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's a more realistic example we could use, since a typical R function like this wouldn't show messages.
Maybe we could add something like:
if (!is.numeric(x)) {
stop("'x' must be a numeric vector")
}
to validate that x
is a numeric vector?
@@ -15,7 +17,7 @@ fivenum <- function(x) { | |||
i <- c(n5, n4, n3, n2, n1) | |||
|
|||
# compute quartile values | |||
x <- sort(x) | |||
x <- base::sort(x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems kind of arbitrary; I think the preview code we use should reflect "real" code as much as possible, and I don't think prefixing with base::
is common. I'm not sure if there's a better alternative. I think it's okay if we omit usages of ::
in the preview here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a default of stats::rnorm(100)
, but if you are not convinced, that's okay, we can omit this.
I also added decreasing = FALSE
to highlight how logical are shown
src/gwt/src/org/rstudio/studio/client/workbench/prefs/views/AppearancePreferencesPane.R
Outdated
Show resolved
Hide resolved
src/gwt/src/org/rstudio/studio/client/workbench/prefs/views/AppearancePreferencesPane.R
Outdated
Show resolved
Hide resolved
…pearancePreferencesPane.R
Intent
When previewing themes, it is not possible to preview how an inline function call looks or a string.
Since some themes show a different color for namespaced functions.
Also added a message to show the color of strings.
Approach
Modify template added in #13560
Automated Tests
N/A visual only when previewing themes
QA Notes
Documentation
NA
Checklist
NEWS.md
Signed agreement