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

Allow contributions of custom CSS properties to CSS extension #47775

Closed
octref opened this issue Apr 12, 2018 · 25 comments
Closed

Allow contributions of custom CSS properties to CSS extension #47775

octref opened this issue Apr 12, 2018 · 25 comments
Assignees
Labels
css-less-scss Issues and items concerning CSS,Less,SCSS styling feature-request Request for new features or functionality on-testplan verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@octref
Copy link
Contributor

octref commented Apr 12, 2018

The motivation is that today, css Language Service would suggest that you have invalid properties.

microsoft/vscode-css-languageservice#68

image

Although by adopting MDN data we should solve the problem, however if new properties come up CSS LS can still fall behind.

We should provide a config [css/less/scss].validate.acceptedProperties or something similar that stops CSS LS from complaining about "unknown property".

@octref octref added the css-less-scss Issues and items concerning CSS,Less,SCSS styling label Apr 12, 2018
@octref octref added this to the April 2018 milestone Apr 12, 2018
@octref octref self-assigned this Apr 12, 2018
@octref
Copy link
Contributor Author

octref commented Apr 13, 2018

@aeschli Actually today we already have a config css.lint.unknownProperties, you can set it to ignore to suppress this error. Can you link to original issues that describe the issue?

@usernamehw
Copy link
Contributor

Isn't this setting ignores all properties? For instance, this will pass:

body {
    teapot: yes;
}

@octref
Copy link
Contributor Author

octref commented Apr 13, 2018

@usernamehw My worry is that we are offering too much surface area for linting settings, when all we offer is a subset of https://github.com/CSSLint/csslint and now we are forking with some more options...

When we have MDN data for properties it'll actually solve all three existing "unknown properties" issues:

@usernamehw
Copy link
Contributor

Then this issue is not really needed.

@octref
Copy link
Contributor Author

octref commented Apr 13, 2018

Yea looking for @aeschli’s input

@aeschli
Copy link
Contributor

aeschli commented Apr 16, 2018

@octref yes, you can use "less.lint.unknownProperties" to turn off all validation warnings.
But the idea is to just be able to pass a list accepted properties, so you can still get validation for all others.
You still want to get a warning when there's a typo in the name. But you want to get rid of that error on 'appearance' that is not yet a standardized property but well understood by your cssnext post-processor. Given that post-processors become more and more common, custom properties getting more used to.

@octref
Copy link
Contributor Author

octref commented Apr 16, 2018

@aeschli The MDN data contains experimental properties such as appearance. CSSNext only implements what's already in Recommendation Candidate, by which point MDN would already have a data point on it.

I suggest that we pull in the MDN data, see if that addresses people's complaint

Given that post-processors become more and more common, custom properties getting more used to.

If the need arises we can always add that. But I think if someone is using a property not known on MDN (as compared to now known on our outdated css-schema), it's fair we output a warning.

@shockthetoast
Copy link

Would it be possible to fix the type in the title of this issue? As it is, it doesn't show up in a search for "unknown property". It would help those of us searching for this exact issue know it is being worked on. Thanks!

@octref octref changed the title Allow config for custom properties in CSS so VS Code don't complain "unkonwn property" Allow config for custom properties in CSS so VS Code don't complain "unknown property" Apr 18, 2018
@octref
Copy link
Contributor Author

octref commented Apr 18, 2018

@shockthetoast Done, thanks!

@octref octref modified the milestones: April 2018, Backlog Apr 23, 2018
@octref
Copy link
Contributor Author

octref commented Sep 10, 2018

This is no longer an issue — using latest data from MDN so we don't have "unknown properties" any more.

@octref octref closed this as completed Sep 10, 2018
@octref octref modified the milestones: Backlog, September 2018 Sep 10, 2018
@octref octref added feature-request Request for new features or functionality css-less-scss Issues and items concerning CSS,Less,SCSS styling and removed css-less-scss Issues and items concerning CSS,Less,SCSS styling labels Sep 10, 2018
@carlosrovira
Copy link

Hi, I'm searching for a way to add custom properties to css.lint, since in our project (Apache Royale) we have custom properties related only to our project, since I'm using VSCode, I'd love to have some way to add custom properties and even create some extension for it. Thanks for any advice on this.

@aeschli aeschli reopened this Sep 19, 2018
@aeschli aeschli modified the milestones: September 2018, Backlog Sep 19, 2018
@aeschli
Copy link
Contributor

aeschli commented Sep 19, 2018

I also think we should have this for custom properties.

@carlosrovira
Copy link

Great,

I think so, since CSS is open its capabilities and used in many new ways that can't be controlled.
Here's a screenshot of a typical Apache Royale use case:

captura de pantalla 2018-09-19 a las 16 23 24

I expect that we can remove the css lint error configuring in some way a config file. Even add this config file to extensions like AS3 & MXML Extension that already manages Apache Royale projects so we can catch error typing this kind of custom css properties.

Thanks!

@connorshea
Copy link
Contributor

@connorshea
Copy link
Contributor

So I've gotten a decent way through making this work, but I can't figure out the correct way to pass the custom property configuration into the place it's needed.

I see this line elsewhere in the vscode-css-languageservice repo, but I can't figure out how to pass a settings object to the languageFacts.ts file in a similar way.

The docs say to use vscode.workspace.getConfiguration('myExtension'), but I can't get the vscode module imported into the vscode-css-languageservice extension for whatever reason. I just end up with Cannot find module 'vscode'. (this is after adding vscode as a dependency in the package.json, installing, and restarting).

I suppose it's also important to note that I added the configuration to the package.json for vscode/extensions/css-language-features, rather than vscode-css-languageservice, because all other existing configuration options were there already.

Any guidance on how I should pass the user's configuration into vscode-languageservice's languageFacts.ts? I can share my current code if that'd help.

@carlosrovira
Copy link

Hi connor, thanks for working on this. Just to understand the progress. Are you implementing some kind of property file where we can add the custom css properties we want to be recognized by vscode lint, and get rid of lint errors? (event get code-intelligence for those custom properties defined in that property file? Thanks!

@connorshea
Copy link
Contributor

@carlosrovira Right now it's a WIP so it's hard-coded until I can figure out how to pass the configuration into it, but it looks like this:

screen shot 2018-10-02 at 2 59 56 pm

Currently the description isn't able to be customized, it's just set as "Custom property", but that can probably be changed relatively easily if you'd really like to be able to do that.

The properties would be defined in a VS Code settings.json file, which could be user-specific or workspace-specific (e.g. you'd have a file at .vscode/settings.json and it defines various custom properties you use in that project), like this:

{
  "css.customProperties": [
    "custom-property-name",
    "another-custom-property",
    "custom-property-three"
  ]
}

@octref
Copy link
Contributor Author

octref commented Oct 2, 2018

@connorshea It would be better start working after discussing with me or @aeschli. I don't think we'll go with that approach (more likely going with the direction of the Markdown Extension API https://code.visualstudio.com/docs/extensionAPI/api-markdown), specify a JSON format and add contribution points.

This issue is only for adding a setting to make VS Code not complain about unknown properties.

@carlosrovira
Copy link

Hi @octref,
as a user; right is to make VS Code not complain about unknown properties, but as well, providing code-intelligence of the custom properties will be a huge plus and will make this very usable for the community.

Thanks.

@connorshea
Copy link
Contributor

connorshea commented Oct 2, 2018

@octref

@connorshea It would be better start working after discussing with me or aeschli.

Fair enough, that's my bad :)

This issue is only for adding a setting to make VS Code not complain about unknown properties.

A setting to make VS Code not complain about unknown properties already exists as you mentioned above ("css.lint.unknownProperties": "ignore"), the issue description does mention a [css/less/scss].validate.acceptedProperties setting, though, which is what I was trying to implement.

Also, just to clarify, a "CSS Extension API" would be preferable because it would allow extensions to add CSS properties, or am I missing other reasons?

@octref octref changed the title Allow config for custom properties in CSS so VS Code don't complain "unknown property" Allow contributions of custom CSS properties to CSS extension Oct 2, 2018
@octref
Copy link
Contributor Author

octref commented Oct 2, 2018

Oops, the title tricked me, updated.

Also, just to clarify, a "CSS Extension API" would be preferable because it would allow extensions to add CSS properties, or am I missing other reasons?

As compared to saving everything in the project config, I think putting all the custom CSS property definitions in a JSON and allow a setting/contribution to refer to it would be better. This keeps clutter away from workspace config and allows others to auto-generate the JSON file.

@aeschli
Copy link
Contributor

aeschli commented Oct 3, 2018

@octref I'm not against allowing users to define sets of properties in a separate file, but IMO a simple user setting is a good start/ The css language service doesn't deal with files anyways, it's just a library, also to be used in Monaco. All validation configuration comes in through config objects, so LanguageSettings is the right place.
Where this gets relevant is in the server that uses the service. Let's discuss how we proceed once we are there.

@Connormiha The validation happens here:
https://github.com/Microsoft/vscode-css-languageservice/blob/83462f41b87303ffc32b58b714a18806da389182/src/services/lint.ts#L413

Here you would say, only report error of not a known property or not in the list.

@carlosrovira
Copy link

Also, just to clarify, a "CSS Extension API" would be preferable because it would allow extensions to add CSS properties, or am I missing other reasons?

I expect the AS3 & MXML(http://as3mxml.com/) extension could add a file so we have our custom properties manages directly by the extension that provide all the language support.

Thanks

@aeschli
Copy link
Contributor

aeschli commented Oct 29, 2018

I added a new setting that allows specifying 'valid' properties that are not verified.

fixed by microsoft/vscode-css-languageservice@e0a7d58

@aeschli aeschli closed this as completed Oct 29, 2018
@aeschli aeschli assigned aeschli and unassigned octref Oct 29, 2018
@aeschli aeschli modified the milestones: Backlog, October 2018 Oct 29, 2018
@aeschli aeschli added verification-needed Verification of issue is requested on-testplan labels Oct 29, 2018
@mjbvz mjbvz added the verified Verification succeeded label Oct 31, 2018
@octref
Copy link
Contributor Author

octref commented Dec 2, 2018

FYI: #64164

@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
css-less-scss Issues and items concerning CSS,Less,SCSS styling feature-request Request for new features or functionality on-testplan verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants