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

[feat] Error message for inline component style directive #7187

Merged
merged 5 commits into from
Feb 2, 2022
Merged

[feat] Error message for inline component style directive #7187

merged 5 commits into from
Feb 2, 2022

Conversation

efeskucuk
Copy link
Contributor

@efeskucuk efeskucuk commented Jan 25, 2022

This is for the feature request #7177 . Passes all the tests, fails with 2 lint errors that are unrelated to my changes. I've added one test.

@bluwy bluwy linked an issue Jan 26, 2022 that may be closed by this pull request
@Conduitry
Copy link
Member

"not currently supported" implies that we're considering this feature but just haven't gotten around to it. Is that accurate? Would we want a style: directive on a component to, say, wrap it in a display: contents div and apply the styles to that, like we do with style props currently?

@efeskucuk
Copy link
Contributor Author

"not currently supported" implies that we're considering this feature but just haven't gotten around to it. Is that accurate? Would we want a style: directive on a component to, say, wrap it in a display: contents div and apply the styles to that, like we do with style props currently?

I've wrote the error message after reading this comment,
#7176 (comment)

Let me know if you want it to be changed to something else.

@bluwy
Copy link
Member

bluwy commented Jan 26, 2022

Hmm. I assumed it could be a considered feature, since the original error message was Not implemented: StyleDirective, which sort of implied that it could be implemented. But I'd agree this new error message now could be misleading. We can have it as Style directives cannot be used on components instead.

@Conduitry
Copy link
Member

Not implemented: StyleDirective is just a generic error coming from

throw new Error(`Not implemented: ${node.type}`);
and doesn't indicate anything about whether this is a possible feature.

@efeskucuk
Copy link
Contributor Author

I've changed the message to what @bluwy suggested.

@dummdidumm dummdidumm merged commit 587f94e into sveltejs:master Feb 2, 2022
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.

Friendlier Errormessage when trying to use style directive on components
4 participants