Skip to content
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

feat: add combined styles and inbuilt syntax highlighting themes in the REPL #2341

Merged
merged 2 commits into from
Jun 10, 2024

Conversation

Snehil-Shah
Copy link
Member

@Snehil-Shah Snehil-Shah commented Jun 8, 2024

Description

What is the purpose of this pull request?

This pull request:

  • adds the ability to combine styles and colors. styles like bold brightBlue, italic magenta are now supported.
  • adds the following inbuilt themes: 'stdlib-ansi-basic', 'stdlib-ansi-dark', 'stdlib-ansi-light', 'stdlib-ansi-strong', 'minimalist', 'monokai', 'solarized'.

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

One of the challenges is finding out how accessible these themes are across different terminal emulators. How these ANSI colors are rendered depends on the terminal application. In my system, all of the terminal applications render all ANSI colors according to accessibility standards. So it was difficult for me to judge which color is suitable for dark/light backgrounds. If most modern emulators do this then this might be a win for us.

It would be great if you could try out the themes on your terminals and suggest changes that make the themes more accessible.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@Planeshifter Planeshifter self-requested a review June 8, 2024 18:39
@kgryte kgryte added Enhancement Issue or pull request for enhancing existing functionality. REPL Issue or pull request specific to the project REPL. labels Jun 8, 2024
@kgryte
Copy link
Member

kgryte commented Jun 8, 2024

@Snehil-Shah How did you access accessibility compliance?

@Snehil-Shah
Copy link
Member Author

@kgryte I should have clarified, just visually looking at, if it's clearly visible or not. If there's a way to assess accessibility, let me know..

@kgryte
Copy link
Member

kgryte commented Jun 8, 2024

I suppose one comment regarding the styles is that, for themes such as solarized and monokai, in IDEs, this also sets the application background. For example, in SublimeText, if you set monokai, the IDE background is not fully black. For solarized, there is a similar background adjustment. Here, it's just the tokens.

@kgryte
Copy link
Member

kgryte commented Jun 8, 2024

just visually looking at, if it's clearly visible or not. If there's a way to assess accessibility, let me know.

Yes, one should use color contrast checkers, such as https://webaim.org/resources/contrastchecker/.

@Snehil-Shah
Copy link
Member Author

@kgryte Yes, but tmk there isn't a way to do that in terminal environments, so I just tried to match the token colors through visual judgement for monokai and solarized themes..

@kgryte
Copy link
Member

kgryte commented Jun 8, 2024

Ideally, themes have full AAA compliance, but this may not always be possible, especially if a theme has many colors. In which case, some colors having AA compliance is okay.

@Snehil-Shah
Copy link
Member Author

Yes, one should use color contrast checkers, such as https://webaim.org/resources/contrastchecker/.

@kgryte As mentioned in the OP, that's the problem, generic ANSI color names don't have a fixed hex code attached AFAIK, so it depends on the terminal emulator on how they are rendering these colors. On my terminals, the current themes look fine.. but not sure if that's true for other emulators.

@kgryte
Copy link
Member

kgryte commented Jun 8, 2024

Re: background. Yeah, you are correct. Seems no real portable way to do this (e.g., https://unix.stackexchange.com/questions/474502/how-to-set-the-background-color-of-the-linux-console-screen). Up to a user to cycle through themes to determine which looks "best".

On that note, maybe we should add commands for easily cycling through themes: prevTheme() and nextTheme(). These should only be REPL commands, and don't need to be exposed on the REPL.prototype.

@kgryte
Copy link
Member

kgryte commented Jun 8, 2024

After playing around with it locally, seems like we need to figure out the terminal color story more broadly in order to craft accessible themes. The limited ANSI color palette doesn't provide enough range. Apart from some of our stdlib-* themes, the other themes could benefit from more colors.

@kgryte
Copy link
Member

kgryte commented Jun 8, 2024

Another command that may be nice for seeing how themes render is to provide something similar to how our example() command is supposed to work (note: I believe this is broken atm due to various recent REPL changes). Namely, it outputs a series of constructs/expressions as example code solely for the purpose of showing how a theme will render. Currently, after switching themes, the only way to see the differences is by re-entering previous commands. In IDEs, they have the advantage that they just re-render everything. I suppose, in principle, we could do the same thing, but would likely be tricky to pull off as would require getting the current contents of the screen, finding commands, reparsing, replacing various escape codes, and then re-rendering. May be just easier to provide a "lorem ipsum" kind of command for helping users see theme differences.

@kgryte
Copy link
Member

kgryte commented Jun 8, 2024

The sample content would not have to be commands which run. Instead, could be something along the lines of

In [1]: themeSample()
// This is a comment...
var x = 1;
if ( x > 0 ) {
    console.log( 'foo' );
}
function bar() {
    var o = { 'beep': 'boop' }
}
// ...

where the return value is a template which gets filled in with ANSI escape codes. Meaning, instead of actually executing code, the return value is syntax highlighted, as would be the case if entered as commands.

var tmpl = [
	'{{BEGIN_COMMENT}}// This is a comment...{{END_COMMENT}}',
	'{{BEGIN_IDENTIFIER}}var{{END_IDENTIFER}} {{START_VARIABLE}}x{{END_VARIABLE}} = {{BEGIN_LITERAL}}1{{END_LITERAL}};'
	// ...
];

// ...

log( repl, render( tmpl, theme ) );

// ...

@Snehil-Shah
Copy link
Member Author

Snehil-Shah commented Jun 9, 2024

@kgryte

  • With prevTheme() & nextTheme(), the user is still required to execute a command to change the theme, which they were doing previously as well. And yes while they can click the up arrow to bring back the command again, they can also bring back the command settings('theme', 'name') and just edit out the name. All I'm trying to propose is, this approach isn't making cycling through themes that much easier. Instead I feel, certain key bindings for cycling through themes does it better (after we add support for keybindings). So, a user can use settings to set a theme, or if they want to quickly cycle through, they can use a keyboard shortcut.

  • themeSample(), we can have that, although I wonder if it's a bit of an overkill to have a command just to check how the tokens render. Instead maybe we could fuse this with the settings('theme'), command itself, when a user changes a theme, we print the success message and also the sample highlighted code below. A separate command is fine too.

seems like we need to figure out the terminal color story more broadly in order to craft accessible themes.

  • Are you suggesting hex color codes?

@kgryte
Copy link
Member

kgryte commented Jun 9, 2024

My suggestion for prevTheme and nextTheme would be for it to set the theme. It would combine (1) finding the next/previous theme and (2) executing settings( 'theme', '<next/prev theme name>' ).

@kgryte
Copy link
Member

kgryte commented Jun 9, 2024

themeSample: should we find something like this worthwhile, I would not combine with the settings command. I'd make it a separate command, as you don't always want to see sample rendered output.

@kgryte
Copy link
Member

kgryte commented Jun 9, 2024

Are you suggesting hex color codes?

Something to that effect. E.g., https://github.com/chalk/chalk/tree/main, https://github.com/termstandard/colors, https://github.com/chalk/supports-color. If a terminal supports additional colors, we should take advantage of this for better theming. This was discussed previously in a prior PR. Ref: #2254 (comment)

@kgryte
Copy link
Member

kgryte commented Jun 9, 2024

Also, keyboard shortcuts are only a partial solution and will not be guaranteed to be enabled by a user. A command is a lower primitive. A shortcut would ultimately call the command.

@Snehil-Shah
Copy link
Member Author

@kgryte hex colors would require terminal color detection, so we can downsample the provided hex colors to 16 ANSI colors if the terminal doesn't support 256 colors. I think we can reserve this to a future PR after we move colors to a standalone package and have a package for terminal color detection. What do you think?

Regarding the commands, should I include them in this PR itself?

@kgryte
Copy link
Member

kgryte commented Jun 9, 2024

Yes, I think all the above for future discussion and PRs. If you wouldn't mind creating issues for those tasks, that would be great!

Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

Apart from my question, LGTM.

Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Looks fine to me as well.

Let's land it!

@Planeshifter Planeshifter merged commit 0856277 into stdlib-js:develop Jun 10, 2024
6 checks passed
aman-095 pushed a commit to aman-095/stdlib that referenced this pull request Jun 11, 2024
…he REPL

PR-URL: stdlib-js#2341

---------

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Reviewed-by: Athan Reines <kgryte@gmail.com> 
Reviewed-by: Philipp Burckhardt <pburckhardt@outlook.com>
aman-095 pushed a commit to aman-095/stdlib that referenced this pull request Jun 13, 2024
…he REPL

PR-URL: stdlib-js#2341

---------

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Reviewed-by: Athan Reines <kgryte@gmail.com> 
Reviewed-by: Philipp Burckhardt <pburckhardt@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Issue or pull request for enhancing existing functionality. REPL Issue or pull request specific to the project REPL.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants