-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Cell-level errors #1280
Cell-level errors #1280
Conversation
This reduces the need for type assertions, making our code more type-safe and succinct.
This is better because `RequestStatus` stores errors too. If a request has not yet been made (e.g. the 'idle' `State` value), then there will no RequestStatus, i.e. `undefined`.
- Allow iterables in constructor. - Allow removing by array. - Allow a function to merge values of a map when adding.
Changes applied to package-lock.json when restarting docker.
Codecov Report
@@ Coverage Diff @@
## master #1280 +/- ##
=======================================
Coverage 93.46% 93.46%
=======================================
Files 114 114
Lines 4404 4404
=======================================
Hits 4116 4116
Misses 288 288
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
I don't understand why this E2E test is failing. It seems to be timing out.
The listed timeout is not what I'd expect. It should be 30000ms because we have overridden the @silentninja any ideas here? |
Note: I merged changes from #1286 (which is set for auto-merge into master) into this PR in an attempt to get the CI to pass. |
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.
@seancolsen The PR looks good. I noticed a couple regressions we need to fix before merging this in:
- Skeleton does not show when there is a sheet level loading action - initial load, refresh, pagination, applying sorting etc., It should be visible in these scenarios.
- Skeleton shows during a cell update. It should not be visible during this scenario.
We can figure out cell background behavior later.
5a13e3f
to
11900e7
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.
@seancolsen Nice work on this!
I do see another regression:
- When rows are deleted, we re-fetch data for the entire page.
- The skeleton indication is now shown for the entire sheet for this. It should not be shown.
I'm going to merge this in. I'll add a separate issue for this.
Created #1289 |
Fixes #1045
Works towards #775
Demos
Issues
Checklist
Update index.md
).master
branch of the repositoryDeveloper Certificate of Origin
Developer Certificate of Origin