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

Enable customizations of the editor #25

Merged
merged 6 commits into from
Mar 19, 2021

Conversation

ChayimFriedman2
Copy link
Contributor

We provide a settings page, that enables the user to change all of Monaco's options except those that are irrelevant here.

There are two settings that I did not found a good way to express in the settings UI: fontLigatures can be a string or a boolean (currently only a checkbox for boolean), and rulers is an array.

I'm not good with UX and the UI is minimal. If someone more knowledgeable than me would like to style it, I welcome suggestions.

Accessibility isn't good either, and again, if you know better than me what to do, please share it!

This fixes bunch of issues. Here's what I've found:

We provide a settings page, that enables the user to change all of Monaco's options except those that are irrelevant here
Copy link
Owner

@hediet hediet left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution!

The PR looks good on the first glimpse! I will have a more detailed look (hopefully) in the next couple of days.

These two things trouble me:

  • There are a lot of settings with this PR. I guess 80% of them are never changed by any user. This amount of settings makes refactoring the settings UI a lot more work. I think only the requested and the most important settings should be implemented.
  • These settings are passed directly to monaco without validating them. Does JavaScript Prototype Poising apply? What are the risks that the settings go corrupt and monaco receives garbish, making the text editor unusable?

I yet have to build/try out your PR.

src/content-script.ts Outdated Show resolved Hide resolved
options.html Outdated
Comment on lines 9 to 18
<p><label><input type="checkbox" id="scrollBeyondLastLine" data-setting="true"> Scroll beyond last line</label></p>
<p>
<label>Word wrap <select id="wordWrap" data-setting="true">
<option value="off">Off</option>
<option value="on">On</option>
<option value="wordWrapColumn">Word wrap column</option>
<option value="bounded">Bounded</option>
</select></label>
</p>
<p>
Copy link
Owner

Choose a reason for hiding this comment

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

These are a lot of settings. I think it might make sense to only add requested settings to keep the UI lean, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed that if Microsoft put effort into implementing these settings, there are people that want them.

Copy link
Owner

Choose a reason for hiding this comment

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

I doubt vscode exposes all the those settings. Imo these settings are for programmers to customize Monaco to fit into the product, not necessarily for end users.
I'd love to hear more opinions on that.
My philosophy is usually "less is more", especially if I have to maintain it!

Maybe you can, alternatively, somehow reduce the redundancy in options.html? Ideally each setting should only correspond to a single line of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt vscode exposes all the those settings. Imo these settings are for programmers to customize Monaco to fit into the product, not necessarily for end users.

Not for all settings, but when I was in doubt, I checked if VSCode provides the setting. So you're both right and wrong at the same time 😄.

Maybe you can, alternatively, somehow reduce the redundancy in options.html? Ideally each setting should only correspond to a single line of code.

I had to type all of this stuff, so be sure I've thought about that 😆. I came up with the conclusion that there is nothing I can do to really reduce the number of HTML code here. By data-setting=true and automatically applying the setting based on the element ID, most of the work is done. I can reduce some of the code by custom elements, but I don't think this is worth (The most of the code is from the dropdowns, where I have to specify the options either way).

Edit: I'll try and write the conclusions here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Experience was good and it reduced the number of lines. Pushed.

@ChayimFriedman2
Copy link
Contributor Author

These settings are passed directly to monaco without validating them. Does JavaScript Prototype Poising apply? What are the risks that the settings go corrupt and monaco receives garbish, making the text editor unusable?

I've thought about security, but considering that the settings object is both produced and consumed on the client side, I see no problem. The only way to exploit this messaging is by injecting a malware script into the page, and the only bad thing that can happen is corrupted editor, so that's not a problem.

And BTW, if you can inject a script to the page there are worse thing you can do.

@hediet
Copy link
Owner

hediet commented Mar 3, 2021

@ChayimFriedman2 seems reasonable.

@ChayimFriedman2 ChayimFriedman2 marked this pull request as draft March 3, 2021 14:16
@ChayimFriedman2 ChayimFriedman2 marked this pull request as ready for review March 3, 2021 20:41
@hediet hediet merged commit 33b4b9b into hediet:master Mar 19, 2021
@hediet
Copy link
Owner

hediet commented Mar 19, 2021

Thank you!

@hediet
Copy link
Owner

hediet commented Mar 19, 2021

Btw. I think it would be a nice feature to highlight changed settings in bold.

@ChayimFriedman2
Copy link
Contributor Author

OK. I'm pretty busy this week, but maybe I'll find time to push this.

@ChayimFriedman2 ChayimFriedman2 deleted the settings-ui branch March 20, 2021 18:06
@hediet
Copy link
Owner

hediet commented Mar 20, 2021

Published with v0.5.0!

@bbugh
Copy link

bbugh commented Mar 21, 2021

Thanks for this @ChayimFriedman2!

@hediet
Copy link
Owner

hediet commented Apr 1, 2021

Oops, installation count dropped from 530 to 450 after requesting the "storage" permission!
I think any future permission should be requested dynamically.

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.

None yet

3 participants