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

Bug/remove mini image from admin view #2976

Conversation

nvh0412
Copy link
Contributor

@nvh0412 nvh0412 commented Nov 26, 2018

Description

This PR has been created to fix #2975

  • Remove mini_image helper
  • Use spree/admin/shared/image instead with image from gallery

Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

@nvh0412 Thanks for making a PR!

There are definitely some deletions in here that shouldn't be, do you mind fixing that?

@nvh0412 nvh0412 force-pushed the bug/remove_mini_image_from_admin_view branch from 56acf79 to feb58d6 Compare November 26, 2018 16:18
@jacobherrington
Copy link
Contributor

jacobherrington commented Nov 26, 2018

It looks like you picked up 42d6c9c by accident!

@nvh0412
Copy link
Contributor Author

nvh0412 commented Nov 26, 2018

@jacobherrington a mistake with my git tree, I fixed. Thanks so much, jacob 💯

@nvh0412
Copy link
Contributor Author

nvh0412 commented Nov 26, 2018

@jacobherrington seems like we have a flaky test https://circleci.com/gh/solidusio/solidus/10020#tests/containers/1, ironically I can't rerun to make it pass.

@jacobherrington
Copy link
Contributor

@nvh0412 Yes, I believe that test is a bit flaky. It keeps showing up in various PRs.

@nvh0412
Copy link
Contributor Author

nvh0412 commented Nov 27, 2018

@jacobherrington so we have to wait for the fix of this flaky test to merge this PR?

@jacobherrington
Copy link
Contributor

jacobherrington commented Nov 27, 2018

I don't think this PR is related, but I'd like to figure out why the test is red before approving.

Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

@nvh0412
Copy link
Contributor Author

nvh0412 commented Nov 29, 2018

@jacobherrington did anyone fix the flaky test?

@jacobherrington
Copy link
Contributor

@nvh0412 In this case, no, I just reran the test suite. That test is not failing because of your PR, it needs to be addressed as its own issue.

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks!

@jacobherrington jacobherrington merged commit c6c3b15 into solidusio:master Dec 1, 2018
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.

Undefined method mini_image
3 participants