-
Notifications
You must be signed in to change notification settings - Fork 214
feat: document NpmPackage npm assets #4513
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
Conversation
Add documentation on copying npm assets for NpmPackage. part of vaadin/flow#21806
AI Language Review
|
update example
|
|
||
| [since:com.vaadin:vaadin@V24.9] | ||
| [#fonts-and-images-from-npm-with-npmpackage-annotation] | ||
| == Loading Fonts and Images from npm Packages Using the NpmPackage annotation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No mention of CSS? I'm wondering because this would also replace the theme.json's importCss, or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't thought about that, but you could of copy any css as an asset and then have a @CssImport for it to load the css.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll test around a bit and add the information to the document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be out of scope.. but I wanted to mention it because the ticket from above says "In Vaadin 25 we are going to deprecated Theme annotation and features in theme.json, including importCss instruction that then needs a replacement." and I was wondering where my importCss replacement is :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a quick test it would seem that we don't need anything new as the following seems to work
@NpmPackage(value = "@fortawesome/fontawesome-free", version = "5.15.1")
@CssImport("@fortawesome/fontawesome-free/css/all.css")
@Route("theme")
public class ThemeView extends Div {
public ThemeView() {
Span sharp = new Span();
sharp.addClassNames("fa-sharp","fa-solid","fa-user");
sharp.getStyle().set("font-family", "'Font Awesome 5 Free'");
add(sharp);
}
}
But this might need some extra testing to see that it wasn't just a fluke or something in the test module enabling this..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking! Previously people could do it also like this:
{
"parent": "my-base-theme",
"importCss": ["@fortawesome/fontawesome-free/css/all.min.css"],
"lumoImports": ["typography","color","spacing","badge","utility"]
}
| The assets from the [annotationname]`NpmPackage` annotation are copied to the static folder `VAADIN/static` instead of `VAADIN/static/themes/[themeName]/` | ||
|
|
||
| In the sample case the `fortawesome` file `svgs/regular/snowflake.svg` is copied to `VAADIN/static/fontawesome/icons/snowflake.svg` and should be called by that path in the application and css. | ||
| This difference is due to the custom theme call having the `themes/[themeName]/` in the request letting us know it is a theme resource, making it possible to prepend the static part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ThemeResource still a thing, even after the deprecation of the ´@Theme` annotation? Also, talking about a “custom theme” might be problematic, referring to the old theme mechanics, rather than just regular stylesheets.
| This difference is due to the custom theme call having the `themes/[themeName]/` in the request letting us know it is a theme resource, making it possible to prepend the static part. | |
| This difference is due to the custom theme call having the `themes/[themeName]/` in the request letting Vaadin know it's a theme resource, making it possible to prepend the static part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theme annotation will be deprecated along the themes/[themeName] path but it's still supported. After deprecation, "theme resource folder" is relevant only for apps still using it. For those it's "custom theme" and for new apps it's just regular css.
This article could have a reference to https://vaadin.com/docs/latest/styling/advanced/npm-packages#styles-from-npm to make it clearer what "custom theme" mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see the text before talking about the difference. But since this is the documentation for Vaadin 25, I don't think we should mention deprecated APIs at all, but guide users to use just the supported APIs.
I would remove any mentions and comparisons to theme.json, custom themes and theme resources. @peholmst, would you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Don't mention anything deprecated. Only mention what's current and recommended.
Co-authored-by: Jouni Koivuviita <jouni@vaadin.com>
# Conflicts: # articles/styling/advanced/npm-packages.adoc
jouni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried my best by editing this directly on GitHub. I removed mentions to theme.json, and focused only on documenting the @NpmPackage annotation and the assets field, and the @CssImport annotation.
* feat: document NpmPackage npm assets Add documentation on copying npm assets for NpmPackage. part of vaadin/flow#21806 * minor chages/clarifications update example * Add info on npm stylesheet with cssimport * Update articles/styling/advanced/npm-packages.adoc Co-authored-by: Jouni Koivuviita <jouni@vaadin.com> * Remove mentions to old feature fron new feature. # Conflicts: # articles/styling/advanced/npm-packages.adoc * edits * Update npm-packages.adoc --------- Co-authored-by: Tomi Virtanen <tltv@vaadin.com> Co-authored-by: Jouni Koivuviita <jouni@vaadin.com> Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
* feat: document NpmPackage npm assets Add documentation on copying npm assets for NpmPackage. part of vaadin/flow#21806 * minor chages/clarifications update example * Add info on npm stylesheet with cssimport * Update articles/styling/advanced/npm-packages.adoc * Remove mentions to old feature fron new feature. # Conflicts: # articles/styling/advanced/npm-packages.adoc * edits * Update npm-packages.adoc --------- Co-authored-by: caalador <mikael.grankvist@vaadin.com> Co-authored-by: Tomi Virtanen <tltv@vaadin.com> Co-authored-by: Jouni Koivuviita <jouni@vaadin.com> Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
Add documentation on copying
npm assets for NpmPackage.
part of vaadin/flow#21806