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

ui: Adds multi syntax linting to the code editor #4814

Merged
merged 2 commits into from
Oct 19, 2018

Conversation

johncowen
Copy link
Contributor

This PR adds multi syntax linting capabilities to the code editor.

The linting/syntax javascript files are loaded on demand, so if you don't need a certain syntax, it won't be loaded. This gives us the freedom to add more syntaxes in the future with no downsides.

screen shot 2018-10-18 at 16 01 00

screen shot 2018-10-18 at 16 01 13

Currently we've included:

  1. JSON: Syntax highlighting and linting
  2. HCL: Syntax highlighting only
  3. YAML: Syntax highlighting and linting

The look and feel is yet to be refined, although as it is functionally 'ready', I thought people might appreciate this in the upcoming release. It also might prompt users to give us any other syntaxes they might need including.

The syntax mode loader is a little 'out of the ordinary' as it tries to use the existing CodeMirror loadMode plugin to avoid reinventing the wheel.

The syntax highlighting colors themselves keep the colors we followed from the vault ui.

Fixes #4671 , #4258 and #4256

@johncowen johncowen added the theme/ui Anything related to the UI label Oct 18, 2018
@johncowen johncowen requested a review from a team October 18, 2018 15:12
@johncowen johncowen added this to the 1.4.0 milestone Oct 19, 2018
@johncowen
Copy link
Contributor Author

P.S. I've had second thoughts about re-using the codemirror service.

I don't own it so I shouldn't be adding/mixing things into it. Hence the second best option is yet another service just for dealing with syntax/modes/linting. It's going to be a bit weird as it will be very specific to code mirror, but I think I prefer that. 🤔 Another option would be to extend the existing code mirror service instead and have like a second service('code-mirror/lintable') instead, at least then its clear that it has some extras.

In my mind, I don't think this would prevent this from going into a release soon if possible as it's more dev hygiene than anything.

Copy link
Contributor

@pearkes pearkes left a comment

Choose a reason for hiding this comment

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

Looks good! I couldn't find anything surprising from my view. May be worth getting another set of 👀 but think this will be great.

@johncowen johncowen changed the base branch from feature/ui-acl-v2 to release/1.4-staging October 19, 2018 15:30
@johncowen johncowen changed the base branch from release/1.4-staging to feature/ui-acl-v2 October 19, 2018 15:36
@banks banks force-pushed the feature/ui-multi-linting branch from 86c2db6 to 615af57 Compare October 19, 2018 16:28
@banks banks changed the base branch from feature/ui-acl-v2 to master October 19, 2018 16:29
@johncowen johncowen merged commit d98c124 into master Oct 19, 2018
@johncowen johncowen deleted the feature/ui-multi-linting branch October 19, 2018 16:36
@johncowen johncowen mentioned this pull request Nov 3, 2021
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants