Skip to content

Conversation

Akash-IAV
Copy link
Contributor

Description

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for committer review:

  • DEPENDENCIES are up-to-date. Dash license tool. Committers can open IP issues for restricted libs.
  • Copyright and license header are present on all affected files
  • If helm chart has been changed, the chart version has been bumped to either next major, minor or patch level (compared to released chart).

Copy link
Contributor

@tom-rm-meyer-ISST tom-rm-meyer-ISST left a 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 and frontend/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 in package-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.

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)!

if (handleDelete) {
return [
...columns,
{
Copy link
Contributor

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.

@Akash-IAV
Copy link
Contributor Author

Thanks for the detailed suggestions, Tom!

  • As per your feedback, I’ll review the implementation in #1004 and try to apply a similar approach to our modal — starting with the stock modal.

  • I've also created two separate branches for these changes and for the changes related to #1005 as you suggested.

  • I’ll make the necessary changes in the new branch accordingly and raise a separate PR for this.

@Akash-IAV Akash-IAV closed this Oct 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants