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

Allow string in 'root_sass' configuration #54

Merged

Conversation

smnandre
Copy link
Contributor

@smnandre smnandre commented Jan 2, 2024

Allow both string and array to configure root_sass. (follows discussion here

symfony_cast:
    root_sass: '%kernel.project_dir%/assets/scss/app.scss'
symfony_cast:
    root_sass: 
        - '%kernel.project_dir%/assets/scss/app.scss'

Magic done in the dependency extension, so it does not impact any domain code

@smnandre
Copy link
Contributor Author

smnandre commented Jan 2, 2024

Update: still think the documentation / recipes should contains the array version, as it's easier to understand that multiple values are allowed, and how to add them :)

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

Easy win, thanks!

still think the documentation / recipes should contains the array version, as it's easier to understand that multiple values are allowed, and how to add them

Why not use both? :) I think we can show "string" example first (as it will fit most users), but below mention you can specify more values with an array. Wdyt?

@smnandre
Copy link
Contributor Author

smnandre commented Jan 2, 2024

Of course, even better :)

Just anticipated issues/requests "how can i use multiple pathes" elsewhen :)

@makraz
Copy link
Contributor

makraz commented Jan 2, 2024

Easy win, thanks!

still think the documentation / recipes should contains the array version, as it's easier to understand that multiple values are allowed, and how to add them

Why not use both? :) I think we can show "string" example first (as it will fit most users), but below mention you can specify more values with an array. Wdyt?

@bocharsky-bw, @smnandre, I update my PR #52 to update the docs with the array version :)

@bocharsky-bw
Copy link
Member

Thank you!

@bocharsky-bw bocharsky-bw merged commit 83f4e5d into SymfonyCasts:main Jan 3, 2024
11 checks passed
@smnandre
Copy link
Contributor Author

smnandre commented Jan 3, 2024

I just discover this could be written in a cleaner / shorter way:

->beforeNormalization()->castToArray()->end()

CF: https://symfony.com/doc/current/components/config/definition.html#array-nodes

Should i update the code ?

@makraz
Copy link
Contributor

makraz commented Jan 4, 2024

I just discover this could be written in a cleaner / shorter way:

->beforeNormalization()->castToArray()->end()

CF: https://symfony.com/doc/current/components/config/definition.html#array-nodes

Should i update the code ?

@smnandre I can do it you don't mind?

@smnandre
Copy link
Contributor Author

smnandre commented Jan 4, 2024

Of course you can :)

@makraz
Copy link
Contributor

makraz commented Jan 4, 2024

I just discover this could be written in a cleaner / shorter way:

->beforeNormalization()->castToArray()->end()

CF: https://symfony.com/doc/current/components/config/definition.html#array-nodes

Should i update the code ?

the PR is here: #55

cc: @smnandre @bocharsky-bw

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.

3 participants