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.

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

[make:stimulus-controller] New Stimulus controller Maker command #1075

Merged
merged 3 commits into from
May 4, 2022
Merged

[make:stimulus-controller] New Stimulus controller Maker command #1075

merged 3 commits into from
May 4, 2022

Conversation

JabriAbdelilah
Copy link
Contributor

@JabriAbdelilah JabriAbdelilah commented Feb 20, 2022 •
edited
Loading

New Command to generate Stimulus Controller with targets and values.

Stimulus controller Maker command

test_controller.js

import { Controller } from '@hotwired/stimulus';

/*
* The following line makes this controller "lazy": it won't be downloaded until needed
* See https://github.com/symfony/stimulus-bridge#lazy-controllers
*/
/* stimulusFetch: 'lazy' */
export default class extends Controller {
    static targets = ['errors']
    static values = {
        maxItems: Number,
    }
    // ...
}

Tickets #809

@JabriAbdelilah JabriAbdelilah mentioned this pull request Feb 20, 2022
Closed
@JabriAbdelilah JabriAbdelilah force-pushed the stimulus-controller-maker branch 2 times, most recently from 1d1d8f0 to 5177550 Compare February 20, 2022 16:00
@JabriAbdelilah JabriAbdelilah changed the title New Stimulus controller Maker command New Stimulus controller Maker command make:stimulus-controller Feb 20, 2022
@JabriAbdelilah JabriAbdelilah changed the title New Stimulus controller Maker command make:stimulus-controller New Stimulus controller Maker command Feb 20, 2022
94noni
94noni reviewed Feb 21, 2022
View reviewed changes
src/Maker/MakeStimulusController.php Outdated Show resolved Hide resolved
src/Resources/skeleton/stimulus/Controller.tpl.php Show resolved Hide resolved
src/Maker/MakeStimulusController.php Outdated Show resolved Hide resolved
jrushlow
jrushlow requested changes Feb 23, 2022
View reviewed changes
Copy link
Collaborator

@jrushlow jrushlow 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 looking real good @JabriAbdelilah! a couple of comments below:

src/Maker/MakeStimulusController.php Outdated Show resolved Hide resolved
src/Maker/MakeStimulusController.php Outdated Show resolved Hide resolved
src/Maker/MakeStimulusController.php Outdated Show resolved Hide resolved
src/Maker/MakeStimulusController.php Outdated Show resolved Hide resolved
src/Resources/skeleton/stimulus/Controller.tpl.php Show resolved Hide resolved
src/Resources/skeleton/stimulus/Controller.tpl.php Outdated Show resolved Hide resolved
tests/Maker/MakeStimulusControllerTest.php Outdated Show resolved Hide resolved
@jrushlow jrushlow added the Status: Needs Work Additional work is needed label Feb 23, 2022
@JabriAbdelilah JabriAbdelilah force-pushed the stimulus-controller-maker branch from a8782eb to a963522 Compare February 26, 2022 19:24
@JabriAbdelilah
Copy link
Contributor Author

@jrushlow @94noni @zairigimad can you re-check, please?

@JabriAbdelilah JabriAbdelilah force-pushed the stimulus-controller-maker branch from a963522 to b4dae5f Compare February 26, 2022 19:29
94noni
94noni reviewed Feb 26, 2022
View reviewed changes
src/Resources/help/MakeStimulusController.txt

<info>php %command.full_name% hello</info>

If the argument is missing, the command will ask for the controller name interactively.
Copy link
Contributor

Choose a reason for hiding this comment

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

The extension also isnt it?

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 I forget to update this file, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@94noni in fact, the extension is interactive, no need to add it in here

@JabriAbdelilah JabriAbdelilah force-pushed the stimulus-controller-maker branch from b4dae5f to 7238a67 Compare February 27, 2022 18:22
@JabriAbdelilah
New make Stimulus controller
9086819 
update test

Coding standards

Coding standards

Make constants private

Add targets and language

Respeect Coding Standards

Possibility to add values

remove extra line
@JabriAbdelilah JabriAbdelilah force-pushed the stimulus-controller-maker branch from 7238a67 to 9086819 Compare February 27, 2022 18:24
94noni
94noni approved these changes Mar 2, 2022
View reviewed changes
Copy link
Contributor

@94noni 94noni left a comment

Choose a reason for hiding this comment

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

really nice addition

94noni
94noni reviewed Mar 2, 2022
View reviewed changes
src/Resources/skeleton/stimulus/Controller.tpl.php
<?php if ($values) { ?>
static values = {
<?php foreach ($values as $value): ?>
<?= $value['name'] ?>: <?= $value['type'] ?>,
Copy link
Contributor

Choose a reason for hiding this comment

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

just I think the last , is to much no? I mean after the latest value type is written

Copy link

Choose a reason for hiding this comment

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

That's a trailing comma, it's a good practice in Javascript

@jrushlow jrushlow added the Feature New Feature label Mar 3, 2022
@JabriAbdelilah
Copy link
Contributor Author

@jrushlow this PR is ready for review

@jrushlow jrushlow changed the title New Stimulus controller Maker command [make:stimulus-controller] New Stimulus controller Maker command May 4, 2022
@jrushlow jrushlow added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Work Additional work is needed labels May 4, 2022
jrushlow
jrushlow approved these changes May 4, 2022
View reviewed changes
Copy link
Collaborator

@jrushlow jrushlow left a comment

Choose a reason for hiding this comment

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

Thank you @JabriAbdelilah for the new maker!

@jrushlow jrushlow merged commit 9b96324 into symfony:main May 4, 2022
@jrushlow jrushlow mentioned this pull request May 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AbdelilahJabri AbdelilahJabri AbdelilahJabri left review comments

@94noni 94noni 94noni approved these changes

@OskarStark OskarStark OskarStark left review comments

@jrushlow jrushlow jrushlow approved these changes

@maxhelias maxhelias maxhelias approved these changes

@zairigimad zairigimad zairigimad approved these changes

@dunglas dunglas dunglas approved these changes

Assignees
No one assigned
Labels
Feature New Feature Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@JabriAbdelilah @dunglas @OskarStark @94noni @zairigimad @maxhelias @jrushlow @AbdelilahJabri