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

Playground autocomplete #77

Closed
wants to merge 7 commits into from
Closed

Playground autocomplete #77

wants to merge 7 commits into from

Conversation

jamesnw
Copy link

@jamesnw jamesnw commented Aug 8, 2024

Description

Adds Sass-specific Autocomplete

Related Issue(s)

#78

Steps to test/reproduce

Go to Playground, type in Sass-specific words.

  1. At-rules, like @use, @debug, etc.
  2. Built in module names- @use "sass:math", for instance
  3. Built in module variables only if module is imported- math.$pi
  4. Built in module functions only if module is imported- math.clamp
  • Miriam check functionality
  • Stacy check functionality
  • Jonny review code

Copy link

netlify bot commented Aug 8, 2024

Deploy Preview for sass-site-oddbird ready!

Name Link
🔨 Latest commit b2937f2
🔍 Latest deploy log https://app.netlify.com/sites/sass-site-oddbird/deploys/66da0f36690acb00089d7199
😎 Deploy Preview https://deploy-preview-77--sass-site-oddbird.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jamesnw jamesnw linked an issue Aug 8, 2024 that may be closed by this pull request
@jamesnw
Copy link
Author

jamesnw commented Sep 3, 2024

@mirisuzanne and @stacyk - could you try out the Autocomplete here and see if it works well and as expected?

Copy link
Member

@mirisuzanne mirisuzanne left a comment

Choose a reason for hiding this comment

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

  1. At-rules: works great
  2. Built in module names: only works with e.g. "sass:math", not 'sass:math'. Should probably work with single-quotes as well?
  3. Built in variables: works great
  4. Built in functions: works great

@jamesnw
Copy link
Author

jamesnw commented Sep 4, 2024

2. Built in module names: only works with e.g. "sass:math", not 'sass:math'. Should probably work with single-quotes as well?

Good catch! Fixed.

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

@jamesnw Works great! Obviously it would be ideal if we could get some of these values (e.g. at-rules, modules, etc) dynamically from Sass itself. I assume you looked into that and they're not being exposed currently? It might even be worth mentioning that to Natalie to see what she thinks.

Comment on lines +10 to +27
const atRuleKeywords = [
'use',
'forward',
'import',
'mixin',
'include',
'function',
'extend',
'error',
'warn',
'debug',
'at-root',
'if',
'else',
'each',
'for',
'while',
];
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add @media, @supports, and @keyframes here? And maybe also add autocomplete for other CSS at-rules?

Copy link
Member

Choose a reason for hiding this comment

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

I think css at-rules would be good to consider supporting. Hopefully there's a clear enough division between that and auto-completing css properties/values, so this doesn't become a very slippery scope slope.

Copy link
Author

Choose a reason for hiding this comment

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

I think that probably should go upstream into @codemirror/lang-css- that shouldn't be too challenging, I don't think.

We are already getting auto-completion of css properties and values from that package.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense -- add a comment or file a new issue so we don't lose track?


const builtinModules: ModuleDefinition[] = [
{
name: 'color',
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we're not including the color functions (e.g. adjust, scale, etc) here?

Copy link
Author

Choose a reason for hiding this comment

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

I'm holding out hope there's a way to get this from Sass directly, as this is a long list, and will be changing once the CSS Color 4 changes are made in Sass.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense -- add a comment so we don't forget?

source/assets/js/playground/autocomplete.ts Outdated Show resolved Hide resolved
source/assets/js/playground/autocomplete.ts Show resolved Hide resolved
source/assets/js/playground/autocomplete.ts Show resolved Hide resolved
source/assets/js/playground/autocomplete.ts Outdated Show resolved Hide resolved
source/assets/js/playground/autocomplete.ts Outdated Show resolved Hide resolved
@jgerigmeyer jgerigmeyer removed their assignment Sep 5, 2024
@stacyk
Copy link
Member

stacyk commented Sep 5, 2024

@jamesnw Will this story (or any future stories) include autocompleting CSS properties? For instance, in the SCSS panel, if I type

.foo {
  di

I was expecting to see display as an option but instead I only see html elements

Screenshot 2024-09-05 at 12 27 18 PM

@jamesnw
Copy link
Author

jamesnw commented Sep 5, 2024

@jamesnw Will this story (or any future stories) include autocompleting CSS properties? For instance, in the SCSS panel, if I type

.foo {
  di

I was expecting to see display as an option but instead I only see html elements

The issue is the missing ; on the first line is messing with the parser, and it can't determine the context-

image

@stacyk
Copy link
Member

stacyk commented Sep 5, 2024

@jamesnw I'm not sure the full scope of this story or if it is even unexpected, but I noticed when I type @use and select sass:math but realize a second later I wanted sass:maps, if I delete a few characters from the end it won't give me the same options. After doing a bit more testing I see I can't delete that final quote. Is that the expected user behavior? I thought it should auto complete with sass:* suggestions without that final quote.

@stacyk
Copy link
Member

stacyk commented Sep 5, 2024

@jamesnw Will this story (or any future stories) include autocompleting CSS properties? For instance, in the SCSS panel, if I type

.foo {
  di

I was expecting to see display as an option but instead I only see html elements

The issue is the missing ; on the first line is messing with the parser, and it can't determine the context-

image

I'm literally LOL

@jamesnw
Copy link
Author

jamesnw commented Sep 5, 2024

@jamesnw I'm not sure the full scope of this story or if it is even unexpected, but I noticed when I type @use and select sass:math but realize a second later I wanted sass:maps, if I delete a few characters from the end it won't give me the same options. After doing a bit more testing I see I can't delete that final quote. Is that the expected user behavior? I thought it should auto complete with sass:* suggestions without that final quote.

This is another instance of confusing the parser. Without the end quote, the parser doesn't know it's a StringLiteral, and thinks it's a pseudoclass. I pushed a change that should address it.

Copy link
Member

@stacyk stacyk left a comment

Choose a reason for hiding this comment

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

The update you made for the autocomplete without the final quote works great!

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

👏

@@ -11,7 +11,7 @@ import {
historyKeymap,
indentWithTab,
} from '@codemirror/commands';
import {css as langCss} from '@codemirror/lang-css';
import {css as langCss, cssCompletionSource} from '@codemirror/lang-css';
Copy link
Member

@jgerigmeyer jgerigmeyer Sep 5, 2024

Choose a reason for hiding this comment

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

Unused import?

Suggested change
import {css as langCss, cssCompletionSource} from '@codemirror/lang-css';
import {css as langCss} from '@codemirror/lang-css';

Comment on lines +10 to +27
const atRuleKeywords = [
'use',
'forward',
'import',
'mixin',
'include',
'function',
'extend',
'error',
'warn',
'debug',
'at-root',
'if',
'else',
'each',
'for',
'while',
];
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense -- add a comment or file a new issue so we don't lose track?


const builtinModules: ModuleDefinition[] = [
{
name: 'color',
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense -- add a comment so we don't forget?

@jamesnw
Copy link
Author

jamesnw commented Sep 5, 2024

Moving to sass repo- sass#1166

@jamesnw jamesnw closed this Sep 5, 2024
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.

Playground- autocomplete
4 participants