Skip to content

Conversation

@dennisvankekem
Copy link
Collaborator

does need some work as the red lines appear only on line 1, didn't manage to get a solution in time, monaco-yaml seemed like a promised solution but required ejecting react and tinker with web pack which is a no go. If someone knows an elegant solution please let me know. Otherwise I'll try to finish it up next monday

@j-zimnowoda j-zimnowoda self-requested a review August 30, 2024 13:17
Copy link
Contributor

@j-zimnowoda j-zimnowoda left a comment

Choose a reason for hiding this comment

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

It does not seem to work:

  • I was able to remove all values from editor and it did not complain.
  • I was able to add additional fields and it also did not complain

There if also a validate button that does not fit the styling. I do not think that this button is even needed.

Lastly it would be nice if you follow some standards about creating PRs.

  1. Most of our colleagues create a branch in apl-core where you can easily deploy changes. It is useful especially if changes require other PRs from other repos.
  2. This PR requires PR from apl-api but you have not indicated that.
  3. You have not invited anyone to PR review, why?

image

@dennisvankekem
Copy link
Collaborator Author

@j-zimnowoda this PR was supposed to be a draft PR but it seems that I didn't set it to draft, that is also the reason why I said it needs some work and why there is no reviewers or why it doesn't seem to fit the PR standards.

@dennisvankekem dennisvankekem marked this pull request as draft September 2, 2024 07:25
@dennisvankekem dennisvankekem marked this pull request as ready for review September 6, 2024 15:28
@ElderMatt ElderMatt dismissed j-zimnowoda’s stale review September 17, 2024 12:21

Review was based on draft.

@dennisvankekem dennisvankekem merged commit a390be3 into main Sep 17, 2024
2 checks passed
@dennisvankekem dennisvankekem deleted the dvk-validate-values-editor branch September 17, 2024 12:27
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.

4 participants