-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update admin UI views to work with solidus admin 4.4 #29
Update admin UI views to work with solidus admin 4.4 #29
Conversation
Hey @boomer196 I think @kennyadsl would be more than happy to help you on making the tests work and merge it. |
@boomer196 did you take a look why the tests are failing and did you test against 4.4? |
Tests should be green (they are locally). There may be some rubocop violations, (which I have another branch in my fork that fixes these), but I do not think the CircleCI server for this repo is currently working. Yes, I am currently running/testing my fork in a v4.4.2 store. I am just putting these PRs out here in case they help someone. |
Out of curiosity, does the system tokenize the download or how does it work? |
19d13d6
to
078ee20
Compare
DEPRECATION WARNING: Using the "spree/admin/shared/_product_sub_menu" partial is deprecated, please use MenuItem#children instead.
Solidus admin v4.4 does not use icon links or delete buttons
078ee20
to
62784ee
Compare
@kennyadsl, this PR is now green and ready to go. Please take a look when you have a chance. |
@boomer196 thanks. One thing is not super clear to me. Did you update the color of the legacy admin to match the color of the new admin? If yes, I don't think it's the right approach to make the extension work with the new admin. Instead, we should create the views for the new admin using the new components. |
@kennyadsl , I never saw the old admin, so I am not sure what it used to look like. Here are the changes:
|
@boomer196 this can be closed now, correct? |
I believe it is still a valid UI upgrade. However, I am not sure about older versions of the admin (<v4.4). I will defer to @kennyadsl I am fine either way. I gave maintainers edit permission on the PR, so they can either merge it or close it. I will let it sit for another week or so and then I will close it otherwise. Thanks for the reminder (I forgotten about this PR as I have decided not to use this extension). |
Hey, i thought everything was squashed with your merged PR. If it's ok we clone your branch and finish it up. |
@kennyadsl looks good |
product_sub_menu
.fa-icon-cloud
should have beenfa-cloud
)Screenshots
Before
After