-
Notifications
You must be signed in to change notification settings - Fork 74
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
Fixes #22969: Rewrite angular app "ComplianceMode" in Elm #4867
Fixes #22969: Rewrite angular app "ComplianceMode" in Elm #4867
Conversation
-- All our data types | ||
-- | ||
|
||
type ComplianceMode = FullCompliance | ChangesOnly | ReportsDisabled | UnknownMode |
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.
I don 't think you need an UnknownMode. It should make an error if the value is invalid.
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.
I've renamed UnknownMode to ErrorMode, but I've kept it to make it easier to handle this error case, by displaying a message in the form, preventing it from being saved and displaying an error notification.
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.
GReat !
"full-compliance" -> FullCompliance | ||
"changes-only" -> ChangesOnly | ||
"reports-disabled" -> ReportsDisabled | ||
_ -> UnknownMode |
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 should either be an error, not ignored
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.
I used this "Error" mode to display an error in the form, but the user can still select a new valid report mode and save his changes.
Fixes #22969: Rewrite angular app \"ComplianceMode\" in Elm
PR updated with a new commit |
This PR is not mergeable to upper versions. |
OK, merging this PR |
https://issues.rudder.io/issues/22969