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

Inline Solidus Frontend #2

Conversation

gsmendoza
Copy link
Contributor

@gsmendoza gsmendoza commented Apr 5, 2022

This is actually part of #1, but I've separated these commits from that PR to make the diff of that PR easier to view. For this PR, I'd recommend going through the individual commits instead of viewing the PR diff.

@gsmendoza gsmendoza self-assigned this Apr 5, 2022
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate--inline-frontend branch from c1007e6 to fce3ee9 Compare April 5, 2022 10:15
@kennyadsl
Copy link
Member

👍


# Solidus
Frontend contains controllers and views implementing a storefront and cart for Solidus.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a notice to let new users know that this is not the recommended frontend anymore.

//
//= require spree/backend/turbolinks-integration.js
```
## Testing
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not adding tests to the host application if I'm not wrong. So I think this section should instead be called "Contributing".

Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Thanks, @gsmendoza, I left some minor comments. Also:

  • Could we reconfigure CI to be sure everything is well tied?
  • I think you can remove .dockerdev/ and docker-compose.yml for now.

@waiting-for-dev
Copy link
Contributor

Could we reconfigure CI to be sure everything is well tied?

I saw your comment on #1, so I'm ok with doing that there.

@waiting-for-dev
Copy link
Contributor

I think you can remove .dockerdev/ and docker-compose.yml for now.

Ok, I also saw your checkbox about it in #1. So, if it's working, we can leave it. Did you try to run tests?

@waiting-for-dev
Copy link
Contributor

I'll move those comments to #1, as it's where they belong.

@gsmendoza
Copy link
Contributor Author

Ok, I also saw your checkbox about it in #1. So, if it's working, we can leave it. Did you try to run tests?

Yep, I was able to get docker-compose exec app bin/rspec to pass :)

@gsmendoza
Copy link
Contributor Author

Closing PR since #1 is more up-to-date.

@gsmendoza gsmendoza closed this Apr 27, 2022
@gsmendoza gsmendoza reopened this Apr 28, 2022
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate--inline-frontend branch from fce3ee9 to bb34985 Compare April 28, 2022 06:31
@gsmendoza
Copy link
Contributor Author

Reopening the PR because #2 is too difficult to comment on with its changes.

@gsmendoza gsmendoza force-pushed the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate--inline-frontend branch 4 times, most recently from a448708 to db88c10 Compare June 22, 2022 10:07
Doesn't seem used. Getting the following error when I run it:

```
solidus_frontend-3.1.5/lib/spree/frontend_configuration.rb:4:in `<module:Spree>': uninitialized constant Spree::Preferences (NameError)
```
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate--inline-frontend branch from db88c10 to de6b9e1 Compare June 29, 2022 06:46
Comment on lines +6 to +10
gem 'solidus_api', github: 'solidusio/solidus', glob: '**/*.gemspec'
gem 'solidus_backend', github: 'solidusio/solidus', glob: '**/*.gemspec'
gem 'solidus_core', github: 'solidusio/solidus', glob: '**/*.gemspec'
gem 'solidus_frontend', github: 'solidusio/solidus', glob: '**/*.gemspec'
gem 'solidus_sample', github: 'solidusio/solidus', glob: '**/*.gemspec'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The glob format is a workaround for this issue: "Bundler cannot add multiple gems from the same git source" (rubygems/bundler#7346).

Decouple Gemfile from solidus.gemspec.

We're using the '**/*.gemspec' glob format for the gem statements so
that we can work around this issue:

"Bundler cannot add multiple gems from the same git source"
rubygems/bundler#7346
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate--inline-frontend branch from de6b9e1 to 26be4f7 Compare June 29, 2022 08:43
@gsmendoza gsmendoza merged commit dae732e into master Jun 29, 2022
@kennyadsl kennyadsl deleted the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate--inline-frontend branch June 29, 2022 09:19
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.

None yet

3 participants