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

[Admin] Add product properties admin UI #5926

Conversation

forkata
Copy link
Contributor

@forkata forkata commented Nov 14, 2024

Summary

This change addresses #5857 and introduces a new UI for creating and
editing product properties in the new admin.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

The default behaviour is to use the plural version of the model human
name, which is defined in core as "Property Types", however the
navigation in the new admin has different nomenclature for this model.
This change ensures we use the same language as the navigation for
consistency.
forkata and others added 3 commits November 14, 2024 14:48
Adds the `New` component and renders it on the page. It doesn't actually
work yet!

Co-authored-by: benjamin wil <[email protected]>
Previously this would force the development server to run on 3000, but
it is useful to be able to override that.

Co-authored-by: Harmony Evangelina <[email protected]>
This completes the ability to create a new product property in the new
admin UI.

Co-authored-by: Harmony Evangelina <[email protected]>
@forkata forkata force-pushed the issues/5857/add-product-properties-admin branch from b2ea1b7 to 7a8bbd6 Compare November 14, 2024 22:48
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 83.78378% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.52%. Comparing base (e6e0243) to head (7a8bbd6).
Report is 52 commits behind head on main.

Files with missing lines Patch % Lines
...controllers/solidus_admin/properties_controller.rb 77.77% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5926      +/-   ##
==========================================
- Coverage   89.53%   89.52%   -0.02%     
==========================================
  Files         782      783       +1     
  Lines       17980    18015      +35     
==========================================
+ Hits        16099    16128      +29     
- Misses       1881     1887       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bin/dev Show resolved Hide resolved
@tvdeyen
Copy link
Member

tvdeyen commented Nov 15, 2024

Probably a rebuild of #5892

@forkata
Copy link
Contributor Author

forkata commented Nov 21, 2024

Probably a rebuild of #5892

Oh no! I didn't realize that was happening already 😞 We have this as an issue and on the Project board, but I guess not many people are aware of that.

@jarednorman
Copy link
Member

How do we want to proceed here? The other PR needs a little love, but I'm not sure how much the two differ.

@forkata
Copy link
Contributor Author

forkata commented Nov 28, 2024

@jarednorman I can pull the changes from the other PR and rebase them if the owner of that doesn't come back? Otherwise happy to abandon these changes here, as from what I could tell that PR has everything this has and also the updating done.

@MadelineCollier
Copy link
Contributor

@jarednorman I can pull the changes from the other PR and rebase them if the owner of that doesn't come back? Otherwise happy to abandon these changes here, as from what I could tell that PR has everything this has and also the updating done.

Also happy to collate the two as I'm already in this section of the site if you have better stuff to do @forkata just let me know what you prefer!

@jarednorman
Copy link
Member

Yeah, I'm happy for @MadelineCollier to do whatever needs to be done here and carry this over the finish line.

@forkata
Copy link
Contributor Author

forkata commented Dec 5, 2024

Hey @MadelineCollier, if you are okay taking this over, that would be much appreciated ❤️ I might not have a chance to revisit this work until next week so happy to pass it on to you if you have time!

@MadelineCollier
Copy link
Contributor

For sure, I'll handle it today!

MadelineCollier added a commit to MadelineCollier/solidus that referenced this pull request Dec 5, 2024
This builds off of the excellent work done by @Astr0surf3r in
solidusio#5885 and by @forkata in
solidusio#5926 to build out the
create/edit/update flow for product properties in the new admin.

I've added a request spec that was missing and tweaked a few other
things.
@MadelineCollier
Copy link
Contributor

The work started here has been pulled into #6011. Thanks guys!

MadelineCollier added a commit to MadelineCollier/solidus that referenced this pull request Dec 5, 2024
This builds off of the excellent work started in
solidusio#5885 and in
solidusio#5926 to build out the
create/edit/update flow for product properties in the new admin.

I've added a request spec that was missing and tweaked a few other
things.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants