feat(file-picker): support clearing files (VIV-1995) #1983
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1983 +/- ##
===========================================
Coverage 100.00% 100.00%
===========================================
Files 123 361 +238
Lines 1562 7309 +5747
Branches 108 971 +863
===========================================
+ Hits 1562 7309 +5747
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| /** | ||
| * @internal | ||
| */ | ||
| _dropzone?: Dropzone; |
There was a problem hiding this comment.
Why do we need to expose dropzone?
There was a problem hiding this comment.
Otherwise there is an error when accessing #dropzone from valueChanged
I believe this is because FAST will call valueChanged before the constructor is completed and the private member is not ready
There was a problem hiding this comment.
I pushed a change that reverts it. Seems to work.
The problem is that valuechanged is sent to an event listener, which fires outside the context of the element.
By changing the valueChanged to be an arrow function, it retains the context wherever it is called.
There was a problem hiding this comment.
That's not what's happening though. With your fix valueChange will be initialised at a later point and miss the initial call that causes the error. But I'm happy to accept it like this
| @@ -73,6 +73,7 @@ describe('vwc-file-picker', () => { | |||
| unmountedElement.maxFileSize = 256; | |||
| unmountedElement.maxFiles = 1; | |||
| unmountedElement.accept = '.jpg'; | |||
| unmountedElement.removeAllFiles(); | |||
There was a problem hiding this comment.
Isn't removeAllFiles tested in its own it?
There was a problem hiding this comment.
Yes, this is just to cover the optional chain.
To ensure it doesn't throw when component is unmounted
There was a problem hiding this comment.
Looking at the full test (for some reason it was hidden in the diff), it's a very general test and an unclear spec.
- Would
removeAllFilescause an error in unmounted state at any point? - Did removing and appending the component to the DOM (connecting/disconnecting in WC terms?) cause an error before?
In other words,what piece of code do I delete for the expectation to fail?
There was a problem hiding this comment.
removeAllFiles will throw an error in unmounted state without the optional chain (because dropzone is not initialised)
YonatanKra
force-pushed
the
VIV-1995-clearing-files
branch
from
aadebe5 to
f3f0145
Compare
No description provided.