Skip to content

[AssetMapper] Add docs for importmap:install command #18785

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

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

jmsche
Copy link
Contributor

@jmsche jmsche commented Aug 24, 2023

Hi, this PR adds docs for the new importmap:install command added by symfony/symfony#51351.

your repository.
the ``importmap.php`` file to point to it. You can either commit this directory
to your repository, or ignore the directory in your ``.gitignore`` config file.
If the directory is ignored, you can run the
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it « can » or « must » ?

Copy link
Member

Choose a reason for hiding this comment

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

I think in 6.4, we should change the workflow from "must commit" to "should ignore". Let's rework the language for that - it'll be just like composer install: it's a "must" (assuming you are downloading assets locally).

In fact, would you mind opening a new recipe PR for AssetMapper 6.4 that includes assets/vendor/ in .gitignore?

Copy link
Member

Choose a reason for hiding this comment

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

While merging I did some tweaks according to your comments. See 5909e6b

@OskarStark OskarStark requested a review from weaverryan August 24, 2023 19:26
@OskarStark OskarStark changed the title [AssetMapper] Add docs for the importmap:install command [AssetMapper] Add docs for importmap:install command Aug 24, 2023
@javiereguiluz javiereguiluz merged commit 2990dbe into symfony:6.4 Aug 28, 2023
@javiereguiluz
Copy link
Member

@jmsche thanks for this contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants