-
Notifications
You must be signed in to change notification settings - Fork 13
Added Inline Edit functionality in Stock modal #1006
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
Added Inline Edit functionality in Stock modal #1006
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 raising the PR! Please check out the comments for specifc parts.
Overall it would work that way but is despite our last meeting I would go for a modal. Reasons:
- in case of an validation error, this solution discards changes and sends a notification in the background. That's not very user friendly.
- it does not fit the current look and feel as e.g. done in notifications. Yes it's a second level of modals (we don't want to then increase it further in the future) but has the same look and feel - also with regard to validations.
@ReneSchroederLJ raised pr #1004 to do a technical spike. It's a little bit trickier at some points but actually a lot of the change is refactoring (separation of view and edit modal). As a result we keep the look and feel while keeping the code base the same stile. Thus, please adjust the stock change into that way. Please use the mentioned pr as orientation.
Some overall feedback regarding (the not so well-documented) contribution process:
- Please apply TRG 7.02: If you don't want the IAV to be listed in the license headers, then please comment it that way and no license header updates need to be performed.
- Please keep the versioning up to date:
- keep verison in
backend/pom.xml
,frontend/package.json
andfrontend/package-lock.json
up to date. The material number in the upper part would be patch level and the modal would be minor version. Please always ensure, that you only bump the version according to the latest version released. Note: if you don't just increase the version inpackage-lock.json
and instead also update dependencies, you need to consider TRG 7.04, too. - Update the
./CHANGELOG.md
. If no entry has been created for a new version, create one and copy he known knows, too. If an entry has been created, increase the version number if needed (e.g. current change would be patch level but your change is minor). Always reference the PR in the changelog (this needs either an additional commit or force push (push --force-with-lease
). - bump the chart and the app version in charts/puris/Chart.yaml following the bump above. Update the charts/puris/README.md accordingly (Note: if you add properties you should run helm-docs and then format the table.
- keep verison in
The checkboxes in the pr template are meant to be ticked by the pr requester. They are a little bit of guidance towards TRG 7.04, TRG 7.02 and the versioning strategy. We're currently working on making this implicit knowledge more explicit to improve onboarding for new developers (see #996)!
frontend/src/features/material-details/components/MaterialDetailsHeader.tsx
Outdated
Show resolved
Hide resolved
if (handleDelete) { | ||
return [ | ||
...columns, | ||
{ |
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.
Edit should be included in the same cell as delete, same as for notifications list. Please change.
Thanks for the detailed suggestions, Tom!
|
Description
Pre-review checks
Please ensure to do as many of the following checks as possible, before asking for committer review: