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

Fix: frontend assets should be added to vendor #220

Merged
merged 2 commits into from
Feb 24, 2022

Conversation

gsmendoza
Copy link
Contributor

@gsmendoza gsmendoza commented Feb 21, 2022

Goal

We want the frontend assets and directories added to a Solidus app because
Solidus extensions assume that these files and directories exist. The Solidus
extensions typically link these pre-existing files to their assets.

Expected behavior

Given I have a Solidus app without a frontend

When I apply the SolidusStarterFrontend template to the app

Then I should see the following frontend assets and directories added to the
app:

  • vendor/assets/images/spree/frontend
  • vendor/assets/javascripts/spree/frontend/all.js
  • vendor/assets/stylesheets/spree/frontend/all.css

Current behavior

SolidusStarterFrontend doesn't add the frontend assets and directories to the
app's vendor directory.

Bug cause

Prior to solidusio/solidus#4251, the Solidus Install
generator was adding the frontend assets during installation even if the app
didn't have solidus_frontend in its bundle. With the issue fixed,
SolidusStarterFrontend has to be updated to add these frontend assets by
itself.

Video explanation

https://www.dropbox.com/s/b673ijy66cafx0n/eng-296-update-solidusstarterfrontend-to-add-all.mkv?dl=0

Video: demo of bug and fix

2022-02-22.18-29-18.mp4

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • N/A: New feature (non-breaking change which adds functionality)
  • N/A: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • N/A: My change requires a change to the documentation.
  • N/A: I have updated the documentation accordingly.

@gsmendoza gsmendoza self-assigned this Feb 21, 2022
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-296-solidus-app-use-case-update branch 5 times, most recently from 55d4e35 to 15e4096 Compare February 22, 2022 09:53
Goal
----

We want the frontend assets and directories added to a Solidus app because
Solidus extensions assume that these files and directories exist. The Solidus
extensions typically link these pre-existing files to their assets.

Expected behavior
-----------------

Given I have a Solidus app without a frontend

When I apply the SolidusStarterFrontend template to the app

Then I should see the following frontend assets and directories added to the
app:

* `vendor/assets/images/spree/frontend`
* `vendor/assets/javascripts/spree/frontend/all.js`
* `vendor/assets/stylesheets/spree/frontend/all.css`

Current behavior
----------------

SolidusStarterFrontend doesn't add the frontend assets and directories to the
app's vendor directory.

Bug cause
---------

Prior to solidusio/solidus#4251, the Solidus Install
generator was adding the frontend assets during installation even if the app
didn't have `solidus_frontend` in its bundle. With the issue fixed,
SolidusStarterFrontend has to be updated to add these frontend assets by
itself.

Implementation
--------------

This copies as-is the vendored assets of solidus_frontend from
https://github.com/solidusio/solidus/tree/master/core/lib/generators/solidus/install/templates/vendor/assets.
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-296-solidus-app-use-case-update branch from 15e4096 to 0950fca Compare February 22, 2022 10:18
@gsmendoza gsmendoza marked this pull request as ready for review February 22, 2022 10:40
@gsmendoza gsmendoza merged commit 0b6137e into master Feb 24, 2022
@gsmendoza gsmendoza deleted the gsmendoza/eng-296-solidus-app-use-case-update branch February 24, 2022 08:06
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.

1 participant