gadenbuie / rsthemes

🔮 Full RStudio IDE and Syntax Themes
https://www.garrickadenbuie.com/project/rsthemes/
Other
603 stars 45 forks source link

An RStudio theme for the ambitious #99

Closed eric-hunt closed 3 months ago

eric-hunt commented 1 year ago

Hi @gadenbuie,

I really appreciate this package you have created. It makes RStudio an even more enjoyable IDE to work in.

I'd like to contribute this "port" of the Embark theme. I tried to make code highlighting consistent with what a user might experience using Treesitter in Nvim as this port is already established, however, I've also taken some liberties in altering a few things to be what I consider more readable and/or easier on the eyes.

I've applied a lot of the same flat theme principles as found in the Elm and Serendipity themes in this package, which really helped me figure out the dialog coloring (thanks!).

I hope I'm going about this pull request in the right way. If not, please correct me.

gadenbuie commented 1 year ago

This might be my new favorite theme 🤩 I'll try to review in the next couple of days, but at first glance it looks like a stellar PR. Thanks!

eric-hunt commented 1 year ago

Thank you for taking a look! I actually just noticed one overlooked item, which is the Update Packages dialog. The text is very light on a light background -- that may not be the case on all systems, but that's what I see on MacOS. If you happen to know what needs to be adjusted to fix this, I'm happy to try to find something that works better. From inspection it seemed like it was inheriting from something around .rstudio-themes-flat .dialogContent > table div, but I have yet to be successful in remedying this without affecting other dialogs so I may be messing with a class at too high a level. I'm not great with this stuff 😬 .

gadenbuie commented 1 year ago

I've also noticed -- there are definitely a few places where I'm missing style coverage and its quite jarring, especially in dark mode.

Poking around a little bit, I found something like this could work

.gwt-DialogBox-ModalDialog table[role="presentation"] {
  tr, td, th {
    background-color: #222131;
    color: #CBE3E7;
    border: #222131;
  }
}

Not totally sure where to slot that into the source, if you're interested in poking around and adding it please go ahead, or I can later today or this week.

eric-hunt commented 1 year ago

Using this in my theme file (could maybe be incorporated into source to make it more general, but I'll have to play around with that more):

.gwt-DialogBox {
  &[aria-label="Update Packages"] table[role="presentation"] tbody {
    color: $accent_light !important;
  }
  & table[role="tabpanel"] .gwt-Label {
    color: $rmd_heading_foreground !important;
  }
}

I was able to just target the text in the Package Update dialog using the aria-label attribute, as well as repair overly dark text in the Options dialog with the .gwt-Label class. I don't know if using ARIA attributes like "label" as selectors is good practice, however. I'm sure I will soon stumble upon more dialogs that bug a deep OCD part of my core, at which point this approach could get out of hand. 😆

Perhaps this could be in the source to pull a value from whatever users set in rstheme_dialog_options()..

gadenbuie commented 1 year ago

Awesome! I think that's a totally reasonable approach, but there's a small issue with using the aria-label text: I'm pretty sure those labels get translated to match the system language (when it's supported by the IDE). I'm totally cool with starting this approach and we can refine from here. (It'd be really nice if the IDE had more stable selectors for these things, and maybe if it used a few less table elements 😆 )

You're spot on with the suggestion to use the sass behind rstheme_dialog_options(), which is here: https://github.com/gadenbuie/rsthemes/blob/main/inst/templates/rstudio/_dialog-options.scss. If you'd like to add this to your PR, that'd be great!

eric-hunt commented 1 year ago

Actually, I found a class (enigmatically named) that does the same trick and avoids using aria-label. Do you think this is a more stable option given the translation issue?

.gwt-DialogBox.gwt-DialogBox-ModalDialog.GND-IWGDAY {
  & table[role="presentation"] .GND-IWGDJIC tbody {
    color: lighten($ui_rstudio_background, 20%);
  }
  & table[role="tabpanel"] .gwt-Label {
    color: lighten($ui_rstudio_background, 40%);
  }
}

I put this in the _rstudio-dark.scss file as I only saw this affecting the dark themes, and the changes here aren't really compatible with the already light background in the light themes. The values I chose for lighten were to prevent causing similar problems with the base16 themes that use the default dialog background colors, even when classified as "dark." If a user should choose to override this in their own themes to customize the colors they can do that adding the !important flag. I tried this in my embark.R file and it seems to work.

// in embark.R
.gwt-DialogBox.gwt-DialogBox-ModalDialog.GND-IWGDAY {
  & table[role="presentation"] .GND-IWGDJIC tbody {
    color: $accent_light !important;
  }
  & table[role="tabpanel"] .gwt-Label {
    color: $rmd_heading_foreground !important;
  }
}

Let me know if you want me to change or collapse any of those commits, or perhaps not run make.R and change all the theme files in this PR.

gadenbuie commented 1 year ago

Actually, I found a class (enigmatically named) that does the same trick and avoids using aria-label.

Sadly, those classes are a mirage. They're generated when the IDE is compiled and can change between builds 😞

I put this in the _rstudio-dark.scss file as I only saw this affecting the dark themes, and the changes here aren't really compatible with the already light background in the light themes.

I do think they should go in the rstheme_dialog_options() Sass files because all dark themes use the _rstudio-dark.scss file but not all themes go so far as to style the dialogs. The idea with rstheme_dialog_options() is that it introduces a layer of Sass variables that automatically inherit reasonable values from the current theme but that you can change individually if you want. So in this case, you could probably use $ui_rstudio_dialog_background and $ui_rstudio_dialog_foreground (or any others set here). I'd be happy to add this if you want or I'd be just as happy if you want to take another pass at it.

eric-hunt commented 1 year ago

Ah bummer regarding the mystery classes! I'll have another go at it to add to rstheme_dialog_options(). It has been fun learning a little about Sass/CSS. Who knows.. maybe it'll help me make cooler Shiny apps. 😏

I'm on board with what you're saying now. I didn't really like that it changed colors in the themes that didn't inherently style the dialogs to begin with. Thanks for the guidance. All the table elements do make it pretty crazy to keep track of!

eric-hunt commented 1 year ago

Hi again! Sorry for the delay. I've added this chunk to _dialog_options.scss using the aria-label approach for now:

.gwt-DialogBox {
  &[aria-label="Update Packages"] table[role="presentation"] tbody {
    color: $ui_rstudio_dialog_update_packages_foreground;
  }
  & table[role="tabpanel"] .gwt-Label {
    color: $ui_rstudio_dialog_heading_foreground;
  }
}

I added a new argument @param update_packages_foreground to rstheme_dialog_options() (I hope this is what you meant) and gave it what I considered to be semi-sensible defaults -- but feel free to change these if you'd like:

$ui_rstudio_dialog_update_packages_foreground: if($ui_rstudio_is_dark, lighten($ui_rstudio_dialog_background, 20%), darken($ui_rstudio_dialog_background, 40%)) !default;

It looks pretty good to me for most of those themes that do explicitly set dialog theme elements with rstheme_dialog_options().

I didn't add a new argument for that second component of the _dialog_options.scss addition above, as most of what I was correcting with that were things I considered to be heading text in the many settings dialogs (plus a few other difficult to read help/tooltips) -- e.g. Global Options > Console > Display/Execution/Debugging.. -- so I've just made that default to whatever a user would set with the heading_foreground argument or its default.

Happy to revisit if this isn't what you were hoping for. Thanks!

eric-hunt commented 1 year ago

Just stumbled on another less than optimal font color in the New Project... dialog, which I fixed by changing the table[role=...] selector in _dialog_options.scss to "presentation" instead of "tabpanel":

.gwt-DialogBox {
  &[aria-label="Update Packages"] table[role="presentation"] tbody {
    color: $ui_rstudio_dialog_update_packages_foreground;
  }
  & table[role="presentation"] .gwt-Label {
    color: $ui_rstudio_dialog_heading_foreground;
  }
}

I'm hoping this isn't too heavy handed such that it changes other elements of the .gwt-Label class. :grimacing: I'll keep looking, but I haven't noticed anything weird yet. 🤞🏻

eric-hunt commented 3 months ago

Hi @gadenbuie, Sorry to bug you on this now sort of old thread. I've been using this consistently for the past year or so and haven't noticed anything strange introduced by those small tweaks to the dialog options we discussed. I have another three themes Rosé Pine (Main, Moon, and Dawn) to contribute that would probably also benefit from these tweaks. I can rebase them off this branch or start another pull request, whichever you prefer. You can see some screenshots here. I guess I vibe with purplish themes.. 🟣

gadenbuie commented 3 months ago

Hi @eric-hunt! Sorry this has sat here for so long! I'm having a hard time remembering, is this PR ready to merge as it is now or did we want to do a little more work to it? At this point, I'm happy to merge this (and feel bad I haven't yet)

eric-hunt commented 3 months ago

No worries on the timing. I did make a couple changes to default dialog options in this PR, so it was good to test that out for a while. It seems to agree with all the other themes that elect to style the dialogs (e.g. Material, Elm, Serendipity). It was mostly to adjust the font color for the package update and new project dialogs. I think it's ready to go. Do you want me to bundle these other new themes into this PR also, or do another PR later? Thanks!

gadenbuie commented 3 months ago

Do you want me to bundle these other new themes into this PR also, or do another PR later? Thanks!

Let's merge this and do those in a separate PR. Would you mind adding a bullet to the list of palettes and also a bullet in the NEWS file?

eric-hunt commented 3 months ago

I added a bullet to NEWS (and the palette list), but noticed you might be using fledge to auto-generate NEWS.md, so I apologize if I wasn't supposed to add that there manually. 😬

gadenbuie commented 3 months ago

Thanks! I was using fledge, but I think it makes sense to switch back to the simpler workflow in this repo.