Skip to content
This repository has been archived by the owner on Apr 24, 2024. It is now read-only.

Show warning modal only when the form was changed. #92

Conversation

badnames
Copy link
Member

Make the cancel modal only show up if the user has made form inputs (as mentioned in #35).
For this purpose, all existing form components got an onChange callback, that fires every time the user makes an input.

@markus2330 markus2330 requested a review from buenaflor March 11, 2023 10:06
Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me! A few code comments+tests would be great!

@badnames
Copy link
Member Author

badnames commented Mar 11, 2023

A few code comments+tests would be great!

@markus2330 which newly added code passages need more comments in your opinion?
Regarding tests: I could look into adding UI tests as part of another PR.

@markus2330 markus2330 merged commit 5908346 into ElektraInitiative:master Mar 12, 2023
@markus2330
Copy link
Contributor

@markus2330 which newly added code passages need more comments in your opinion?

The comment was not specific to your code. Its only that now you dive in the code, you have a fresher perspective, which might lead to better documentation, than you will have in a few weeks.

Regarding tests: I could look into adding UI tests as part of another PR.

I was mainly thinking about unit tests. @aidnurs was also interested in UI tests. We first need a decision about which UI test framework we use.

@badnames
Copy link
Member Author

The comment was not specific to your code. Its only that now you dive in the code, you have a fresher perspective, which > might lead to better documentation, than you will have in a few weeks.

Ok I understand. I will look into documenting the frontend either today or tomorrow.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants