-
Notifications
You must be signed in to change notification settings - Fork 3
Feat:WD-20929: Make the list of products configurable through UI #234
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
Feat:WD-20929: Make the list of products configurable through UI #234
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.
Thanks for adding this!
I found a few issues and left a couple of suggestions for improvements
static/client/components/EditProductPanel/_EditProductPanel.scss
Outdated
Show resolved
Hide resolved
static/client/components/EditProductPanel/ProductActionModal.tsx
Outdated
Show resolved
Hide resolved
static/client/components/EditProductPanel/_EditProductPanel.scss
Outdated
Show resolved
Hide resolved
static/client/components/EditProductPanel/_EditProductPanel.scss
Outdated
Show resolved
Hide resolved
static/client/components/EditProductPanel/ProductActionModal.tsx
Outdated
Show resolved
Hide resolved
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.
Thanks for the changes!
A couple of more suggestions and then we're good to merge
static/client/components/EditProductPanel/ProductActionModal.tsx
Outdated
Show resolved
Hide resolved
static/client/components/EditProductPanel/ProductActionModal.tsx
Outdated
Show resolved
Hide resolved
static/client/components/EditProductPanel/ProductActionModal.tsx
Outdated
Show resolved
Hide resolved
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.
Thanks for the changes, LGTM 🚀
…gurable-through-ui
| .p-form__group.p-form-validation { | ||
| min-width: 40rem; | ||
| } |
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.
@immortalcodes These styles will target all form controls with these specific classes, even where undesired. To prevent this, always wrap the style within a custom-classed container.
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.
Also, I realized you did this to increase the width of the modal. Instead of increasing the width of input field to make the modal wider, you should target the modal directly
.p-product-modal .p-modal__dialog {
min-width: 40rem;
}|
🎉 This PR is included in version 1.1.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |

Done
Note
QA
QA steps
docker run -d -p 5432:5432 -e POSTGRES_PASSWORD=postgres postgres docker run -d -p 6379:6379 redisyarn devdotrunFixes
Screenshots
It could be helpful to provide some screenshots to aid in QAing the change.