SS-9745: Add web mcp#334
Conversation
|
@MajorMeerkatThe3rd I won't have much time in the next two days to review this, please do a review and tag me if there is something I should look at. |
@snabela No worries, on my list for today |
MajorMeerkatThe3rd
left a comment
MajorMeerkatThe3rd
left a comment
There was a problem hiding this comment.
@LemToUp I did an initial review pass, but stopped to look deeper at some point as the same issues very repeating. Generally, I found these major issues at the moment
- duplicate code: there are many duplications of code at the moment, and I'm not sure why
- separate definitions: there are new type checks that are almost the same as the type checks in appbuildertypechecks and I don't know why.
- low-level checks: there are some checks like isColorObject and I don't understand why they exist. This is a general issue, I'm not even sure why the colorParameterValueValidator exists. Just so you know, via the viewer API every parameter has an "isValid" function. This should be sued instead and makes things much easier.
So I think a lot of this can be simplified. But maybe I misunderstood something. If that is the case, let me know :)
@snabela FYI
|
@MajorMeerkatThe3rd many thanks for review. |
MajorMeerkatThe3rd
left a comment
MajorMeerkatThe3rd
left a comment
There was a problem hiding this comment.
@LemToUp I think this is already much better! I left a few comments that need clarification, thank you
# Conflicts: # features/appbuilder/config/appbuildertypecheck.ts # features/ecommerce/config/ecommerceapitypecheck.ts
MajorMeerkatThe3rd
left a comment
MajorMeerkatThe3rd
left a comment
There was a problem hiding this comment.
@LemToUp Thank you, good to go now as a first version! As you said, we'll need to extend this either way in the future
3 participants
https://shapediver.atlassian.net/browse/SS-9745