Skip to content

Add a basic Symfony UX Stimulus demo #1284

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 21, 2022
Merged

Add a basic Symfony UX Stimulus demo #1284

merged 1 commit into from
Dec 21, 2022

Conversation

94noni
Copy link
Contributor

@94noni 94noni commented Dec 14, 2021 •
edited
Loading

Cf a Slack of mine in the Symfony Slack, I discussed to add a simple/basic Stimulus example leveraging Symfony UX

As requested; this leverages:

symfony-demo-stimulus.mov

Feel free to comment, thx

@stof
Copy link
Member

stof commented Dec 14, 2021

should use Symfony UX 2.0 (and so Stimulus 3)

@94noni
Copy link
Contributor Author

94noni commented Dec 14, 2021 •
edited
Loading

@stof indeed I've just saw that https://symfony.com/blog/symfony-ux-2-0-and-stimulus-3-support
I will try to update underlying packages and finish this accordingly, thanks !
edit: PR updated

GromNaN
GromNaN reviewed Dec 15, 2021
View reviewed changes
public/build/entrypoints.json Outdated
"/build/755.5a8586e9.js",
"/build/login.6b6b5d56.js"
"/build/runtime.js",
"/build/vendors-node_modules_core-js_internals_add-to-unscopables_js-node_modules_core-js_internals_a-029029.js",
Copy link
Member

@GromNaN GromNaN Dec 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Does the generated file names need to be so long? @weaverryan

And the file is 2.57Mb which is too much.

Copy link
Member

Choose a reason for hiding this comment

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

That's a Webpack thing. If you did a "prod" build, the filenames are things like 0.js, 1.js. But in dev mode, Webpack creates long filenames to help give you an idea of what's in there.

About the size: I'm not sure - probably it just has a lot in it. It also, iirc, contains the sourcemap IN the file at the bottom. And, it's unminified because it's a dev build. But we could open up that file to make sure it doesn't contain any huge files that we didn't intend to import.

@94noni 94noni force-pushed the ft-sf-ux-stimulus-demo branch from 44c6b2f to d1d82de Compare December 15, 2021 09:50
@seb-jean
Copy link
Contributor

should use Stimulus / Symfony UX Helper:
https://github.com/symfony/webpack-encore-bundle#stimulus--symfony-ux-helper

@94noni 94noni force-pushed the ft-sf-ux-stimulus-demo branch from d1d82de to e666d49 Compare December 15, 2021 14:33
@94noni 94noni changed the title [WIP] Add a basic Symfony Stimulus demo Add a basic Symfony Stimulus demo Dec 15, 2021
94noni
94noni commented Dec 15, 2021
View reviewed changes
weaverryan
weaverryan reviewed Dec 16, 2021
View reviewed changes
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.

Thanks for this - I think this is a great idea! Other comments:

  • Why did a bunch of public/fonts get committed that weren't there before? That looks unrelated
  • There should not be a package-lock.json - yarn should be used (npm is totally fine, but we tend to use yarn a bit more in Symfony and want to be consistent).

@94noni 94noni force-pushed the ft-sf-ux-stimulus-demo branch from 8fa25f5 to 0fabf86 Compare December 17, 2021 10:15
@94noni
Copy link
Contributor Author

94noni commented Dec 17, 2021 •
edited
Loading

@weaverryan yeah I used npm at first, I will redo with yarn
for my knowledge, what is the command to regenerate the desired assets that are versioned here ?
just yarn run build and I commit all?

edit : second commit is made with yarn run build and git add .

@94noni 94noni force-pushed the ft-sf-ux-stimulus-demo branch from c9fb016 to d28332f Compare December 17, 2021 10:25
@94noni 94noni mentioned this pull request Dec 26, 2021
Closed
@94noni
Copy link
Contributor Author

94noni commented Dec 27, 2021 •
edited
Loading

status: need review

when this PR is reviewed/approved, I will rebase/recompile assets

@94noni
Copy link
Contributor Author

94noni commented Feb 21, 2022

friendly ping @weaverryan @GromNaN @yceruto :)

weaverryan
weaverryan reviewed Feb 22, 2022
View reviewed changes
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.

The JavaScript looks correct to me. I'd say, let's rebase and finish this 👍

@94noni

This comment was marked as resolved.

Sign in to view
@94noni 94noni mentioned this pull request Jul 7, 2022
Closed
@94noni
Copy link
Contributor Author

94noni commented Sep 12, 2022

@weaverryan PR rebased accordingly
cc @javiereguiluz regarding this

@94noni 94noni changed the title Add a basic Symfony Stimulus demo Add a basic Symfony UX Stimulus demo Dec 8, 2022
@javiereguiluz javiereguiluz merged commit ecb6655 into symfony:main Dec 21, 2022
@javiereguiluz
Copy link
Member

Antoine, we finally merged your contribution. I'm really sorry that it took us so long. Thanks for working on this nice feature! Thanks to our lovely reviewers too.

Please note that while merging I faced multiple merge issues. That's why I push forced to your branch.

@94noni 94noni deleted the ft-sf-ux-stimulus-demo branch December 21, 2022 16:27
@94noni
Copy link
Contributor Author

94noni commented Dec 21, 2022

@javiereguiluz thx to you for taking time to handle this, I hope this will help make discover symfony ux and stimulus logic as entry point :)

Please note that while merging I faced multiple merge issues. That's why I push forced to your branch.

yes I may think it was due to versioning assets as the code evolves since

@javiereguiluz javiereguiluz mentioned this pull request Dec 21, 2022
Merged
javiereguiluz added a commit that referenced this pull request Dec 21, 2022
@javiereguiluz
bug #1383 Fix the compiled assets (javiereguiluz)
e4dd387 
This PR was merged into the main branch.

Discussion
----------

Fix the compiled assets

I faced some weird merge issues while merging #1284, so I broke the compiled dependencies. This PR recreates them.

Commits
-------

5576924 Fix the compiled assets
@javiereguiluz javiereguiluz mentioned this pull request Dec 21, 2022
Merged
javiereguiluz added a commit that referenced this pull request Dec 22, 2022
@javiereguiluz
minor #1384 Tweak the password field (javiereguiluz)
018eebc 
This PR was merged into the main branch.

Discussion
----------

Tweak the password field

This tweaks the #1284 feature by changing the button that reveals the password. Instead of an independent button, I propose to integrate it right inside the password field, which I think it's a more common practice:

![password-field](https://user-images.githubusercontent.com/73419/208960860-987eebb0-205e-4746-85ff-9fc2a5fa621e.gif)

Commits
-------

1d779cf Tweak the password field
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GromNaN GromNaN GromNaN left review comments

@yceruto yceruto yceruto requested changes

@weaverryan weaverryan weaverryan approved these changes

Assignees
No one assigned
Labels
Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@94noni @stof @seb-jean @javiereguiluz @weaverryan @GromNaN @yceruto