-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Admin stock locations form #6160
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
Conversation
2acc0de
to
17d8f3d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6160 +/- ##
==========================================
+ Coverage 89.23% 89.31% +0.08%
==========================================
Files 955 959 +4
Lines 20079 20116 +37
==========================================
+ Hits 17917 17967 +50
+ Misses 2162 2149 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2ee470c
to
8c92d69
Compare
526a065
to
95992db
Compare
95992db
to
c0d25ea
Compare
test failure seems to be one of those intermittent |
@kennyadsl @tvdeyen hey guys, you might have missed this one, can I get some eyes 👀 please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract the address component changes into a separate PR please? This change warrants more visibility in changelogs.
c0d25ea
to
aca21dd
Compare
ec015b0
to
fdf9c61
Compare
0f5d2bf
to
09c144d
Compare
@chaimann are there any chances to split up this PR? There are lot of changes in here that are worth it's own changelog entry |
Absolutely! Just to clarify and for the future PRs, what kind of changes would be good candidates to split? Here I assume it should be checkbox component changes. Maybe migration as well? Not sure, as I've seen other PRs where migration was included along with main changes, so I also put it here. Anything else? |
There is no hard rules. Anything that is tangential to the current work or worth a changelog entry because it changes the schema (as in your case) or makes a significant change to a shared component. Again, not a hard rules, more like a rule of thumb and a gut feeling "ah, that might interest others and is worth a special note in the changelog". Anything else in well written commit messages (but you already do that), so that future contributors can make sense of a certain change. In this case I think it is worth to have a dedicated PR about the schema change of spree_stores and maybe the checkbox component, if they are easy to extract. I hope to finish the github actions work soon, so that extra PRs are not that much of a hassle as of right now. Thanks for all the great work on the new admin |
e78e06b
to
4024b3c
Compare
4024b3c
to
dc56e04
Compare
As a common resource controller make it inherit from ResourcesController.
Since stock location form will not be in a modal, change the turbo frame name to be more general :resource_form
Seems like a common fieldset to have, with two fields "phone" and "email"
We don't have to use turbo_frames on these pages since we are not loading content into it but always do a page visit to /new and /edit routes, and when server responds with 422 and templates with validation errors we can just place relevant "id" on the topmost element of the page so its content will be replaced with turbo stream.
dc56e04
to
8f9d2b2
Compare
Summary
Adds a page with form for stock locations create/update, as well as updates stock locations table with links to go to edit page.
Updates "ui/forms/checkbox" component to contain common layout and logic reused between all checkboxes in admin forms, separating a "checkbox" component as a single element into its own component "ui/checkbox".- split to #6275Adds helper methods
.back
,.discard
and.save
for UI button componentsAdds missing tests for refund reasons, tax categories and product forms, where missing hidden input prevented booleans to be saved properly.- split to #6275Page screenshot:


There are a few differences from the design card which I believe make sense to have:
there is noemail
column instock_locations
, and the "Phone" field is included in "ui/forms/address", so "Phone" field is not placed in its own section;email
column is added and phone + email placed in their own address fieldset;Checklist