-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Create and Edit Product Property with UI components issue #5857 #5892
Create and Edit Product Property with UI components issue #5857 #5892
Conversation
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.
@Astr0surf3r You have not followed our pull request guidelines:
- The commit messages lack any explanation.
- Some commits add unnecessary code that gets removed by later commits.
- Linting changes shouldn't be in separate commits and your linting commit also removes unneeded code.
I'd appreciate a rebase of this PR to address these issues.
@MadelineCollier Can you take a pass over this from an implementation perspective and make sure it's consistent with how similar features are built? You have better knowledge of how admin features should be built.
def form_id | ||
dom_id(@property, "#{stimulus_id}_edit_property_form") | ||
end | ||
end |
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.
This file is mostly misindented.
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.
@jarednorman thank you for your message I have just already made rebase in this PR and follow the solidus guideline let me know if need a cleaner PR I can upload 1 single commit with all changes ... let me know please
c399ad5
to
042abb4
Compare
042abb4
to
f52b0cf
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5892 +/- ##
==========================================
+ Coverage 89.54% 89.57% +0.02%
==========================================
Files 783 785 +2
Lines 17994 18054 +60
==========================================
+ Hits 16113 16171 +58
- Misses 1881 1883 +2 ☔ View full report in Codecov by Sentry. |
This commit adds an introductory section telling the reader how the migration process works. It also adds a last section telling the reader to remove any ineligible adjustments from their database. Lastly, it corrects the location of Spree configuration from the `solidus_promotions` initializer to the `spree.rb` initializer.
We don't want to include `solidus_promotions` as a dependency in `solidus` yet, but we do want to release the gem, so it should appear in the all the lists of Solidus sub-gems.
This code has been automatically generated by our 'Prepare release' GitHub action. The actual release is not part of the automation, and it still needs to be manually done by a maintainer.
Due to the top level constant in testing.rake the release task was also trying to release frontend. Using a local variable instead.
This code has been automatically generated by our 'Prepare post-release' GitHub action. Make sure that: - [ ] The new release has been published, along with its tag. See https://github.com/solidusio/solidus/releases/tag/v4.4.0 - [ ] The corresponding patch branch exists. See https://github.com/solidusio/solidus/tree/v4.4 - [ ] The corresponding backport-v4.4 label exists. See https://github.com/solidusio/solidus/labels
5d14ec2
to
1558c9e
Compare
@Astr0surf3r Please rebase this branch wit latest |
f95b07d
to
faee2d4
Compare
faee2d4
to
403c809
Compare
403c809
to
919004b
Compare
919004b
to
9c5577b
Compare
I think this looks reasonable, but can you please rebase this PR against our main branch. I'm not sure what went wrong with your previous rebase, but you've ended up with a bunch of extra commits in your PR that shouldn't be there. |
The work started here has been pulled into #6011. Thanks for your contribution @Astr0surf3r ! |
Summary
fix the issue #5857
Now to add a product property appear a modal
And to edit a Property appear a modal as well
[x] I have written a thorough PR description.
[x] I have kept my commits small and atomic.
[x] I have used clear, explanatory commit messages.
✅ I have added automated tests to cover my changes.
📸 I have attached screenshots to demo visual changes.