Conversation
layershifter
force-pushed
the
docs/use-new-knobs
branch
from
e3ba8b5 to
a8cb613
Compare
Codecov Report
@@ Coverage Diff @@
## master #1224 +/- ##
=======================================
Coverage 72.14% 72.14%
=======================================
Files 731 731
Lines 5596 5596
Branches 1614 1614
=======================================
Hits 4037 4037
Misses 1554 1554
Partials 5 5Continue to review full report at Codecov.
|
…cs/use-new-knobs # Conflicts: # packages/docs-components/src/index.ts # packages/docs-components/src/knobs/KnobInspector.tsx # packages/docs-components/src/knobs/KnobProvider.tsx # packages/docs-components/src/knobs/defaultComponents.tsx # packages/docs-components/src/knobs/types.ts
Generated by 🚫 dangerJS |
layershifter
requested review from
dvdzkwsk,
kuzhelov,
levithomason,
miroslavstastny and
mnajdova
as code owners
| defaultValue: parseValue(props.value), | ||
| unit: `${props.value}`.replace(`${parseValue(props.value)}`, ''), | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
Was taken from KnobScalar 🦅
There was a problem hiding this comment.
Cool, but looks like an unnecessary premature optimisation.
There was a problem hiding this comment.
Actually, no. It computes defaultValue & unit once because defaultValue depends on props.value which will be changed during renders
miroslavstastny
left a comment
miroslavstastny
left a comment
There was a problem hiding this comment.
These are thing you haven't touched in this PR but maybe you can think about them as well:
| defaultValue: parseValue(props.value), | ||
| unit: `${props.value}`.replace(`${parseValue(props.value)}`, ''), | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
Cool, but looks like an unnecessary premature optimisation.
| const LoaderExampleDelay = () => { | ||
| const [mounted] = useBooleanKnob({ name: 'mounted', initialValue: true }) | ||
|
|
||
| return <div style={{ minHeight: 24 }}>{mounted && <Loader delay={500} />}</div> |
There was a problem hiding this comment.
why delay is not a knob?
There was a problem hiding this comment.
Add it there as knob 👍
| children?: (children: React.ReactElement) => React.ReactElement | ||
| } | ||
|
|
||
| const KnobInspector: React.FunctionComponent<KnobInspectorProps> = props => { |
There was a problem hiding this comment.
We should decide whether we prefer FC or FunctionComponent and be consistent with it
There was a problem hiding this comment.
As I remember we agreed on FunctionComponent, @kuzhelov ?
| })} | ||
| </> | ||
| ) | ||
| return props.children ? props.children(children) : children |
There was a problem hiding this comment.
children() confuses me. Can we use render prop?
There was a problem hiding this comment.
I want to follow React API, they use children: https://reactjs.org/docs/context.html#contextconsumer
|
And this package doesn't have |
I don't want to fill default context with something, it should be filled by - components: {} as any,
+ components: {
+ KnobControl: () => null,
+ KnobField: () => null,
+ KnobLabel: () => null,
+
+ KnobBoolean: () => null,
+ KnobRange: () => null,
+ KnobSelect: () => null,
+ KnobString: () => null,
+ },But will it better? 🤔 |
| /> | ||
| ) | ||
| const DropdownExampleLoading = () => { | ||
| const [loading] = useBooleanKnob({ name: 'loading', initialValue: true }) |
There was a problem hiding this comment.
for this specific example, just to make it user friendly in terms of user being able to notice effect, I might suggest to introduce open knob in addition that will be triggered by default. This will make an effect of loading apparent to the client
…eact into docs/use-new-knobs
4 participants
Follow up from #1082.
Replaces old knobs via new context-based. Also, fixes broken examples in
ExternalLayout.