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
#7684: adds error details for multiple selectors and no selectors found errors #7752
#7684: adds error details for multiple selectors and no selectors found errors #7752
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7752 +/- ##
=======================================
Coverage 72.13% 72.14%
=======================================
Files 1268 1269 +1
Lines 39753 39765 +12
Branches 7373 7375 +2
=======================================
+ Hits 28676 28687 +11
- Misses 11077 11078 +1 ☔ View full report in Codecov by Sentry. |
import { type MultipleElementsFoundError } from "@/errors/businessErrors"; | ||
import styles from "./ErrorDetail.module.scss"; | ||
|
||
const MultipleElementsFoundErrorDetail: React.FunctionComponent<{ |
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.
Seems like you could use a single component that takes either a MultipleElementsFoundError or NoElementsFoundError because you're relying on the same property?
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.
Agreed. I should've opened this as a draft and called it out. I kept them separate in case there were any design concerns where they would end up different.
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.
See comment on reusing the error detail component: https://github.com/pixiebrix/pixiebrix-extension/pull/7752/files#r1506105859
When the PR is merged, the first loom link found on this PR will be posted to |
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.
Discussed with @grahamlangford
We should take a more holistic look at error messaging at a later date
What does this PR do?
Discussion
Demo
https://www.loom.com/share/3838c3b3bf644851bae7266df3f68b32
Checklist
src/tsconfig.strictNullChecks.json
(if possible)