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

Add dark theme and improve scrolling experience #552

Merged
merged 12 commits into from
Oct 8, 2024

Conversation

Goju-Ryu
Copy link
Contributor

@Goju-Ryu Goju-Ryu commented Sep 1, 2024

Description:
This PR adds a dark theme to the editor and makes the editor and results elements share a common scrollbar.
There is still quite a bit of hacky stuff going on, but I am trying to read up on the ace editor documentation before doing anything major.

Changes:

  • I have replaces some hard coded colors in the editor.css file with variables to make it easy to switch colors for each theme.
  • A button has been added to the UI for toggling themes.
  • Two svg icons have been added from the open source and MIT licensed library feather icons.
  • The code for calculating offsets in the interpret function in editor.js has been changed a bit to make the input of each part easier to identify in the html, to avoid calling the numbat.interpret function on empty input and similar minor changes.
  • The scroll behavior has been changed to make it so after interpretation only one scrollbar will be present ensuring results and editor always has the same scroll.
  • Fixes some of the error formatting issues

To do's:

  • Fix the carret in the editor not changing color with the theme
  • Fix the highlighting of text in dark mode making the text unreadable
  • Make the gutter image stay in the middle of the screen no matter the number of lines and scroll
  • Make a responsive padding on both sides of screen to move code and results closer together on wider screens
  • Fix the error <input:lineNumber> part of errors being mistaken for an input HTML tag

This change should make it easier to adjust the colors of the themes. Both themes are now defined in only one place, making it easy to change a color for the theme.
@@ -5,33 +5,108 @@ html {
margin: 0;
padding: 0;
height: 100%;

/* Maybe lower font-size to 16px? */
Copy link
Owner

Choose a reason for hiding this comment

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

I'm okay with a slightly smaller font size than 30px but then we probably want to move the code and the results closer together somehow? (e.g. padding on both sides of the screen)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I have set it to 18px while I'm working on it, but I will look into how to make the code and results work with the smaller font later. I will try to make the design responsive to screen size so it will look decent on anything from a phone to a wide display, but it will take some fiddling around for me to get there.

@sharkdp
Copy link
Owner

sharkdp commented Sep 6, 2024

Thank you very much, this looks promising.

The scroll behavior has been changed to make it so after interpretation only one scrollbar will be present ensuring results and editor always has the same scroll.

Nice!

@Goju-Ryu
Copy link
Contributor Author

I can't really figure out which values are appropriate and at which screen sizes, but I have added some responsive margins and change the font depending on the view port size as well. The font size changing, is mostly meant for making it a little better for phones and tablets, but for completeness of the example it also gets larger on larger displays.

Feel free to let me know if I should change the cutoff points for the changes or the font-size/margin values. Other than that, I think it is merge ready unless something is found in review.

@Goju-Ryu Goju-Ryu marked this pull request as ready for review September 16, 2024 18:54
@Goju-Ryu
Copy link
Contributor Author

Goju-Ryu commented Sep 16, 2024

I have worked a bit on correcting the positioning of output when errors occur on an earlier line. I have put my worn on it on a different branch, but I was wondering if you would like it in this PR or as a separate one later on. If you want to see what the changes are about before deciding it can be found here on the numbat-editor-result-positioning branch.

@sharkdp
Copy link
Owner

sharkdp commented Oct 8, 2024

I have worked a bit on correcting the positioning of output when errors occur on an earlier line. I have put my worn on it on a different branch, but I was wondering if you would like it in this PR or as a separate one later on. If you want to see what the changes are about before deciding it can be found here on the numbat-editor-result-positioning branch.

Let's merge this — this is great. If you have further changes, let's look at them in a new PR.

@sharkdp sharkdp merged commit 68e12cf into sharkdp:numbat-editor Oct 8, 2024
15 checks passed
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