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

Remove duplicated Quick start images #3729

Closed
wants to merge 2 commits into from

Conversation

lvillen
Copy link
Contributor

@lvillen lvillen commented Mar 20, 2024

What this PR does / why we need it:

Remove duplicated Quick start images that could potentially be causing an issue with assets handling.

@lvillen lvillen requested a review from mayorova March 20, 2024 10:49
@lvillen lvillen self-assigned this Mar 20, 2024
@jlledom
Copy link
Contributor

jlledom commented Mar 20, 2024

What's the issue and how removing these images help to solve it?

@lvillen
Copy link
Contributor Author

lvillen commented Mar 20, 2024

@jlledom actually this duplicated images is not the cause of the problem, but they're duplicated anyway and we should remove them. Regarding the problem, I'm working on a fix so, if it works, I'll rewrite the PR according to that.

@akostadinov
Copy link
Contributor

Why do we need to copy the images to pack? Isn't it better to keep them where they are?

@@ -64,10 +64,10 @@
"@types/lodash.escaperegexp": "^4.1.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

Better not to add too many unrelated changes or they will make a mess of git's history.

Copy link

This PR is stale because it has not received activity for more than 30 days. Remove stale label or comment or this will be closed in 15 days.

@github-actions github-actions bot added the Stale label Apr 22, 2024
@lvillen lvillen removed the Stale label Apr 22, 2024
@jlledom
Copy link
Contributor

jlledom commented May 7, 2024

What's the problem? How can I reproduce it and check if it's fixed?

@mayorova
Copy link
Contributor

mayorova commented May 7, 2024

What's the problem? How can I reproduce it and check if it's fixed?

The images are not shown in quickstarts (those that have images) in production mode:

image

image

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not related to the PR

Copy link
Contributor

Choose a reason for hiding this comment

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

No related to the PR

Comment on lines +47 to +53
// Handle images coming from the assets folder
environment.plugins.append('Copy', new CopyWebpackPlugin({
patterns: [
{ from: 'app/assets/images', to: 'images' }
]
}));

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this duplicate the images again?

Instead of copying, is there a way to tell webpack to map images/ to assets/images/, and avoid the copy?

@jlledom
Copy link
Contributor

jlledom commented May 9, 2024

Forgot to mention: I tried this branch locally and the error is fixed for me.

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