-
-
Notifications
You must be signed in to change notification settings - Fork 727
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
feat: display removed context props in the UI #6844
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
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.
✅ Code Health Quality Gates: OK
- Improving Code Health: 1 findings(s) ✅
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.
✅ Code Health Quality Gates: OK
- Improving Code Health: 1 findings(s) ✅
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.
✅ Code Health Quality Gates: OK
- Improving Code Health: 1 findings(s) ✅
frontend/src/component/playground/Playground/AdvancedPlayground.test.tsx
Show resolved
Hide resolved
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.
✅ Code Health Quality Gates: OK
- Improving Code Health: 1 findings(s) ✅
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.
✅ Code Health Quality Gates: OK
- Improving Code Health: 1 findings(s) ✅
cb0703f
to
7f7cd71
Compare
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.
✅ Code Health Quality Gates: OK
@@ -28,6 +28,46 @@ const StyledAlert = styled(Alert)(({ theme }) => ({ | |||
marginBottom: theme.spacing(3), | |||
})); | |||
|
|||
const GenerateWarningMessages: React.FC<{ |
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.
ℹ Getting worse: Overall Code Complexity
The mean cyclomatic complexity increases from 4.33 to 4.40, threshold = 4
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.
✅ Code Health Quality Gates: OK
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.
✅ Code Health Quality Gates: OK
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.
Looks nice, just a few comments/questions but non-blockers
frontend/src/component/playground/Playground/AdvancedPlayground.tsx
Outdated
Show resolved
Hide resolved
<GenerateWarningMessages | ||
response={results} | ||
/> |
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 component will be null
when no warning, I'm just wondering if this is ok (i.e. react knows how to handle null components). Even if react supports this, I remember we were using conditional rendering rather than returning null.
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'm pretty sure that react knows how to handle nulls, yeah. I think it's even preferable to, say, a, fragment. ConditionallyRender
also returns null
if it's not rendering anything.
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.
And because in this case, the decision whether to render something or not can be a little complex (though I guess condition={results.warnings?.invalidContextProperties?.length > 0}
would work), I think it makes sense to relegate it to the function instead.
Does that sound reasonable?
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.
Sounds reasonable! I kind of like this approach because it requires less boilerplate code (I mean, tags are boilerplate in a way). So I think it's good and something for my TIL list :)
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.
✅ Code Health Quality Gates: OK
This PR shows context warnings in the UI if they exist.
That means that if you provide invalid properties on the top level, you'll get the result back, but we'll also tell you that we didn't take these properties into account.
Screenie: