Make all container components take containerRef; consolidate tests
#172
Conversation
Codecov Report
@@ Coverage Diff @@
## main #172 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 12 13 +1
Lines 152 158 +6
Branches 53 55 +2
=========================================
+ Hits 152 158 +6
Continue to review full report at Codecov.
|
lyzadanger
force-pushed
the
container-refs
branch
from
8a7d62d
to
1dc9d7f
Compare
| */ | ||
| export function refElement(arg) { | ||
| return /** @type {import('preact').Ref<any>} */ (arg); | ||
| } |
There was a problem hiding this comment.
This explanation is clear although I think the term "downcast" probably best describes what we're doing with the input. I took a shot at creating a generic version of this that has the proper types:
/**
* @template T
* @typedef {import('preact').Ref<T>} Ref
*/
/**
* Helper for downcasting a ref to a more specific type, where that is safe
* to do.
*
* This is mainly useful to cast a generic `Ref<HTMLElement>` to a more specific
* element type (eg. `Ref<HTMLDivElement>`) for use with the `ref` prop of a JSX element.
* Since Preact only writes to the `ref` prop, such a cast is safe.
*
* @template T
* @template {T} U
* @param {Ref<T>|undefined} ref
* @return {Ref<U>|undefined}
*/
export function downcastRef(ref) {
return /** @type {Ref<U>|undefined} */ (ref);
}The above works in place of the current refElement calls, if you adjust the name. The type variables T and U could also be given more descriptive names like Base and Derived.
There was a problem hiding this comment.
This explanation is clear although I think the term "downcast" probably best describes what we're doing with the input.
Heh, this is why I specifically asked for vocabulary when we chatted about this. I'll use your suggestions here.
| /** | ||
| * @typedef {import('preact').ComponentChildren} Children | ||
| * | ||
| * @typedef ContainerProps | ||
| * @prop {Children} children | ||
| * @prop {string} [classes] - Additional CSS classes to apply | ||
| * @prop {import('preact').Ref<HTMLElement>} [containerRef] - Access to the |
There was a problem hiding this comment.
What are the use cases you have in mind for containerRef?
There was a problem hiding this comment.
Immediate use case: https://github.com/hypothesis/frontend-shared/pull/174/files#diff-39adc517b1457f125e1e729d1039b7ce6a3a4c39317a18af693ff418d5cf6073R153
In general, because these are centralized, common components, I want to give authors as much flexibility as possible when using them, and to have a predictable API across them.
| > | ||
| {children} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
An observation - this Actions class is now almost a completely generic version of the <div> in the other components.
lyzadanger
force-pushed
the
container-refs
branch
from
1dc9d7f
to
e6b7b22
Compare
|
@robertknight Have at! I've integrated your version. |
| const containerRef = createRef(); | ||
| createComponent(Component, { containerRef }); | ||
|
|
||
| assert.isTrue(containerRef.current instanceof Node); |
There was a problem hiding this comment.
assert.instanceOf(object, constructor) will give a better error if this check fails.
There was a problem hiding this comment.
Oh, that is a nice assertion to use, thanks.
lyzadanger
force-pushed
the
container-refs
branch
from
e6b7b22
to
65c9079
Compare
Per some verbal discussion today, there is a props API naturally shaping up with some of the container components. There's an immediate need for a
refon theScrollboxcontainer (forTable), so I went ahead and added thecontainerRefprop to all container components. Tests are consolidated here, as well, to DRY out common container tests.