-
Notifications
You must be signed in to change notification settings - Fork 90
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
Voila viewer #2163
Voila viewer #2163
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2163 +/- ##
==========================================
- Coverage 46.31% 46.00% -0.31%
==========================================
Files 446 450 +4
Lines 22179 22328 +149
Branches 2682 2707 +25
==========================================
Hits 10272 10272
- Misses 11001 11145 +144
- Partials 906 911 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 good in general, some typos and nitpicking in the comments
Co-authored-by: Alexei Mochalov <[email protected]>
const endpoint = Config.use().registryUrl | ||
return React.useMemo( | ||
() => `${endpoint}/voila/voila/render/${mkSearch({ url: sign(handle) })}`, | ||
[endpoint, handle, sign], |
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.
with this approach we should make sure that handle
object identity doesnt change, otherwise the url will be regenerated every time new handle object is created, even if its props are unchanged. we can either pass individual props to useMemo
dependencies (like [handle.bucket, handle.key, ...
) or use useMemoEq
since it leverages deep equality checks
})) | ||
|
||
interface ValueBase { | ||
toString: () => string |
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 meant maybe we dont need toString()
and valueOf()
methods here. what do we gain using that instead of plain { key, label }
objects?
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.
We can use any items as long as they have toString
and valueOf
, for example, list of strings or numbers:
<SelectDropdown options={[ 'option1', 'option2' ]} value="option1" />
<SelectDropdown options={[ new Date('2020-01-01'), new Date('2020-01-02') ]} value={new Date('2020-01-01')}/>
Description
TODO
Python: Runbuild.py
for new docstrings