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

codemirror: add config options to style selection #5529

Merged
merged 5 commits into from Nov 21, 2018

Conversation

@futurist
Copy link
Contributor

@futurist futurist commented Oct 26, 2018

Fixes #5528

Below is a demo to use these options:

Screenshots

DEMO

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 30, 2018

cc @tgeorgeux for UX input.

Copy link
Member

@blink1073 blink1073 left a comment

Thanks!

@futurist
Copy link
Contributor Author

@futurist futurist commented Nov 8, 2018

@tgeorgeux Can you review this PR? need any update please kindly let me know.

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Nov 9, 2018

@futurist Sorry to take so long to get on this. First off, thank you for the issue/pull request being organized and easy to read/understand. That's very helpful, but I digress.

I like the styling here, I also think it makes sense to also bring those options into the advanced setting editor.

@blink1073 do you have thoughts? Given these add-ons have not been previously installed I think it makes sense to give people the option to turn them back off.

The relevant files that will need to be updated to include these in the advanced settings editor are:
jupyterlab/packages/codemirror-extension/schema/commands.json
jupyterlab/packages/codemirror-extension/src/index.ts

Otherwise looks great.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 11, 2018

Yeah I think making the highlighting configurable is fair.

@futurist
Copy link
Contributor Author

@futurist futurist commented Nov 14, 2018

@tgeorgeux I think the point is when user switching theme, the selection style should not mess up the theme style? If this is the case, I suggest the default value for these three options should be false, that case nothing changed except for after the user styling their selections, then manually turn on.

@futurist
Copy link
Contributor Author

@futurist futurist commented Nov 20, 2018

@tgeorgeux The latest commit set all these options false as default, please have a check.

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Nov 20, 2018

@futurist I don't mind turning them on by default.

What I was addressing with my earlier comment is to add those options to the code mirror tab in the advanced settings editor. See the screenshot:

screen shot 2018-11-20 at 9 59 21 am

In order to add these options to the advanced settings editor, you will need to build out the relevant schema in these files:
jupyterlab/packages/codemirror-extension/schema/commands.json
jupyterlab/packages/codemirror-extension/src/index.ts

Also, CI stuff isn't my specialty but it looks like the appveyor build is breaking. @blink1073 are tests at 100 right now?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 21, 2018

Appveyor has been flaky...

@futurist
Copy link
Contributor Author

@futurist futurist commented Nov 21, 2018

@tgeorgeux I've pushed new commits, and think below really make sense:

image

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 21, 2018

LGTM, thanks!

@blink1073 blink1073 merged commit 0befb13 into jupyterlab:master Nov 21, 2018
1 of 2 checks passed
@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Nov 21, 2018

Thank you so much @futurist !

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants