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

Highlight strings in Context values #70

Merged
merged 4 commits into from
Sep 27, 2023
Merged

Conversation

mint-thompson
Copy link
Collaborator

Completes task CIMPL-1114.

Try it out with some FSH with lots of Context keywords to see how you feel about these highlights. FHIRPath strings get the usual double-quote style applied. Other contexts don't get any particular style applied. The , token is highlighted, which should be helpful, but might also be distracting? I kind of like it, though.

Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

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

This provides some nice consistency to the Context keyword values. I like the highlighted commas too.

One other thought I had, which is definitely just a question and not something that necessarily has to change, is that Context is now the only keyword that might not have any highlight/change in color. I tried changing some of the "name"s locally just to see how it would look, and I'm actually not sure if I like it, but I wanted to mention it to see if you had already considered it or what you think. I included a screenshot below:

image

This is what it looks like right now with this branch:
image

syntaxes/fsh.tmLanguage.json Outdated Show resolved Hide resolved
jafeltra
jafeltra previously approved these changes Sep 18, 2023
Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

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

The new updates look good to me! ✨

Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

I like it, and it seems to work quite well 99% of the time. I did, however, find that pesky 1% where it doesn't work quite right. If it's tricky to fix, I'm not worried about it; but in case it is easy, I'm letting you know:

CleanShot 2023-09-19 at 09 17 55

@mint-thompson
Copy link
Collaborator Author

@cmoesel I managed to make a small improvement that handles the case you posted. There are still some cases that aren't handled (please don't start your line with a comma), but it's pretty usable.

Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

This looks great, @mint-thompson! Thank you!

@mint-thompson mint-thompson merged commit 4ac76ce into master Sep 27, 2023
14 checks passed
@mint-thompson mint-thompson deleted the cimpl-1114-context-values branch September 27, 2023 14:08
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.

3 participants