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

[Svelte] Introduce Svelte UX component #498

Closed
wants to merge 17 commits into from
Closed

Conversation

ChqThomas
Copy link
Contributor

@ChqThomas ChqThomas commented Oct 9, 2022

Q A
Bug fix? no
New feature? yes
Tickets N/A
License MIT

This PR introduces a UX Svelte component which allows to render Svelte components inside Twig templates, with the ability to pass down props to Svelte components from Twig. It is implemented in the same way as the UX React component and the UX Vue component for coherence.

  1. Install and configure the svelte-loader :
$ yarn add svelte-loader@^3.0.0 --dev

Enable it in your webpack.config.js file :
This part could be simplified by adding a .enableSvelteLoader() method to the webpack-encore API.
Now that symfony/webpack-encore#781 is merged you cas use .enableSvelte()

// webpack.config.js
Encore
    // ...
    .enableSvelte()
;
  1. Call registerSvelteControllerComponents in app.js to register Svelte components
import { registerSvelteControllerComponents } from "@symfony/ux-svelte";

registerSvelteControllerComponents(require.context('./svelte/controllers', true, /\.svelte$/));
  1. Create Svelte "controller components" (eg. Hello.svelte) inside the folder specified above (here assets/svelte/controllers)
// assets/svelte/controllers/Hello.svelte
<script>
    export let name = "Symfony";
</script>

<div>Hello {name}</div>

In the same way than the UX React and UX Vue components, there is a Twig helper function provided by the bundle : the first parameter is the name of your component, the second are the props that will be passed down :

<div {{ svelte_component('Hello', { name: 'Symfony UX' }) }}></div>

There is also an optional third parameter called intro that tells the Svelte component to play transitions on initial render

<div {{ svelte_component('HelloWithTransition', { name: 'Symfony UX with intial transition' }, true) }}></div>

The implementation was heavily inspired by the work from @tgalopin in #329 and from @t-richard in #426 !

@ahmedyakoubi
Copy link

that's really a great PR . as you said this could be simplified by mergin this PR

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

This is great - thanks for this! Before I do a deeper review, could you reorganize the directories to match, for example, the Autocomplete component? I'm talking specifically about having a src/ directory inside of the component and moving the assets into assets/ instead of Resources/assets. We have some inconsistency right now and I'd like to move things to the way that Autocomplete looks. So we should just get this component organized like that from the start :)

@ChqThomas
Copy link
Contributor Author

Reorganization done :)

I have issues setting up multiple tests in the same file with jest, Svelte seems not happy when the DOM is cleared before his $destroy method is called, and it crash when it happens two times in a row, haven't figured out why yet ...

@YummYume
Copy link
Contributor

Hello. Any news on this? This PR looks amazing 😃

@weaverryan
Copy link
Member

Sorry for the delay on this! @ChqThomas could you also update .github/workflows/test.yaml. Currently, new packages need to be added there to trigger them to build in the CI. Currently, your nice tests aren't running :).

fullySpecified: false
}
}
);
Copy link
Member

Choose a reason for hiding this comment

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

Can we use Encore.enableSvelte() now that it exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the new Encore.enableSvelte() works perfectly !

@ChqThomas
Copy link
Contributor Author

Sorry for the delay on this! @ChqThomas could you also update .github/workflows/test.yaml. Currently, new packages need to be added there to trigger them to build in the CI. Currently, your nice tests aren't running :).

@weaverryan Thanks for the review, I added them !
The low-deps test is failing because in webpack-encore-bundle v1.11.0 the stimulus controller renderer was converting boolean values to integer :

Expected : data-symfony--ux-svelte--svelte-intro-value="true"
Actual : data-symfony--ux-svelte--svelte-intro-value="1"

Do you have any idea on how to handle both versions?

@weaverryan
Copy link
Member

You can just bump the minimum-required version of Encore in your composer.json to whatever version fixed this :)

@ChqThomas
Copy link
Contributor Author

Done 👍
If approved, I could take care of the example page for ux.symfony.com in another PR :)

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Looking great!

/.symfony.bundle.yaml export-ignore
/assets/.gitignore export-ignore
/assets/jest.config.js export-ignore
/assets/test export-ignore
Copy link
Member

Choose a reason for hiding this comment

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

Please also add:

/assets/src/*.ts export-ignore

That's a relatively new addition to the packages :)

@@ -0,0 +1,3 @@
branches: ["2.x"]
maintained_branches: ["2.x"]
doc_dir: "src/Resources/doc"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
doc_dir: "src/Resources/doc"
doc_dir: "doc"

"controllers": {
"svelte": {
"main": "dist/render_controller.js",
"webpackMode": "eager",
Copy link
Member

Choose a reason for hiding this comment

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

You can remove this line - it's legacy, so not needed for new stuff.

intro: this.intro,
...payload,
};
this.dispatch(name, { detail, prefix: 'svelte' });
Copy link
Member

Choose a reason for hiding this comment

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

You even noticed this recent change in the other UX packages and updated this PR - I'm very impressed!

<script>
import { fade } from "svelte/transition"

export let name = "without props";
Copy link
Member

Choose a reason for hiding this comment

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

minor, but can we use spaces here instead of tabs?

Svelte components located in the directory ``assets/svelte/controllers`` are registered as
Svelte controller components.


Copy link
Member

Choose a reason for hiding this comment

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

remove the extra line break

Svelte controller components.


You can then render any Svelte controller component in Twig using the ``svelte_component``.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You can then render any Svelte controller component in Twig using the ``svelte_component``.
You can then render any Svelte controller component in Twig using the ``svelte_component()`` function:

The remove For Example: below

*
* @final
*
* @experimental
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the @experimental. I think we'll release this NOT as experimental.

src/Svelte/doc/index.rst Show resolved Hide resolved
/composer.lock
/phpunit.xml
/vendor/
/var/
Copy link
Member

Choose a reason for hiding this comment

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

Is /var/ needed?

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

One small comment for the docs. But I've tested this locally - it's ready to go :)

$ npm install svelte-loader@^3.0.0 --save-dev

# or use yarn
$ yarn add svelte-loader@^3.0.0 --dev
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the version from both of these commands.

Also, add svelte itself, as that will also be needed.

@ChqThomas
Copy link
Contributor Author

Great ! 😁

@ciprian-marius
Copy link

is this ready to be used?

@ChqThomas
Copy link
Contributor Author

is this ready to be used?

Hopefully soon!

@weaverryan Should I start writing the example page for ux.symfony.com ?

@weaverryan
Copy link
Member

@ChqThomas Sorry for the delay! Yes please! I rebased your work over to #789 but will merge very shortly.

Thank you!

@weaverryan weaverryan closed this Apr 14, 2023
weaverryan added a commit that referenced this pull request Apr 14, 2023
This PR was merged into the 2.x branch.

Discussion
----------

[Svelte] Rebased Svelte PR

Just a rebase of the work on #498 by `@ChqThomas` - for some reason, rebasing while merging gave me some trouble, but this was clean :)

Commits
-------

94bc665 [Svelte] Introduce Svelte UX component
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.

None yet

6 participants