Overall type improvements #3707

ghsolomon · 2024-06-28T16:50:19Z

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.

Caught up with develop branch as of last change.
Added CHANGELOG entry, or determined not required.
Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
Updated doc comments / prop-types, or determined not required.
Reviewed and tested on Mobile, or determined not required.
Created Toolbox branch / PR, or determined not required.

If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to
collapse multiple intermediate commits into a single commit representing the overall feature
change. This helps keep the commit log clean and easy to scan across releases. PRs containing a
single commit should be rebased when possible.

Made hoist components limit ref prop only to those setup to handle it.

Had forgotten to remove ref from ElementSpec.

# Conflicts: # CHANGELOG.md

…pe errors

# Conflicts: # CHANGELOG.md # core/HoistComponent.ts # core/HoistProps.ts # desktop/cmp/input/ButtonGroupInput.ts

# Conflicts: # CHANGELOG.md # core/HoistComponent.ts

amcclain · 2024-06-28T21:19:15Z

CHANGELOG.md

@@ -13,15 +13,20 @@

 ## 65.0.0 - 2024-06-26

-### 💥 Breaking Changes (upgrade difficulty: 🟢 TRIVIAL - dependencies only)
+### 💥 Breaking Changes (upgrade difficulty: 🟢 TRIVIAL - dependencies and renamed component prop)


Note that this release has sailed, will need to move these to next CL version entry

amcclain

I only read a little bit of the way through, but looks like a super in-depth and interesting change! I left a few comments with questions - will catch up again when I return after OOO

amcclain · 2024-06-28T21:23:26Z

cmp/badge/Badge.ts

 import {TEST_ID} from '@xh/hoist/utils/js';
 import {splitLayoutProps} from '@xh/hoist/utils/react';
 import classNames from 'classnames';
 import {merge} from 'lodash';
 import './Badge.scss';

-export interface BadgeProps extends HoistProps, BoxProps {
+export interface BadgeProps extends HoistPropsWithRef<HTMLDivElement>, BoxProps {


Realizing that I need to catch up with the ref handling I see we have throughout the toolkit. I had not realized that we were passing refs through on so many components, and I confess I don't know what the rules are here as to when / why we do that (or when/why app components should do the same).

amcclain · 2024-06-28T21:27:10Z

cmp/filter/FilterChooserModel.ts

@@ -145,7 +146,7 @@ export class FilterChooserModel extends HoistModel {
    @observable.ref selectValue: string[];
    @observable favoritesIsOpen = false;
    @observable unsupportedFilter = false;
-    inputRef = createObservableRef<HTMLElement>();
+    inputRef = createObservableRef<HoistInputModel<null>>();


Curious about the null passed to the generic here - what is that about / worth a comment?

I made HoistInputModel generic to provide a type for the inputRef. In cases where that isn't being used, I thought it made sense to pass null since inputRef.current will always be null.

amcclain · 2024-06-28T21:30:30Z

cmp/form/Form.ts

@@ -21,7 +21,7 @@ import {FormModel} from './FormModel';
 /** @internal */
 export interface FormContextType {
    /** Defaults props to be applied to contained fields. */
-    fieldDefaults?: Partial<BaseFormFieldProps> & DefaultHoistProps;
+    fieldDefaults?: Partial<BaseFormFieldProps> & PlainObject;


I'm assuming that use of PlainObject here means that you can put any key and TS will accept. But if you specify a key that is on BaseFormFieldProps we will get some validation on it, right?

Is mobile vs. desktop split the thing that's driving this?

DefaultHoistProps was effectively an alias for PlainObject. Happy to revert this change, but in the process of reviewing some of this with Lee, we talked about the possibility of removing DefaultHoistProps entirely, so lots to consider here! I'm not entirely sure why this needs to be so generous-- might be desktop vs mobile, will investigate.

amcclain · 2024-06-28T21:33:07Z

cmp/layout/Tags.ts

+export const th = elementFactory('th');
+export const thead = elementFactory('thead');
+export const tr = elementFactory('tr');
+export const ul = elementFactory('ul');


Nice cleanup!

amcclain · 2024-06-28T21:35:33Z

cmp/tab/TabModel.ts

@@ -38,7 +39,7 @@ export interface TabConfig {
    icon?: ReactElement;

    /** Tooltip for the Tab in the container's TabSwitcher. */
-    tooltip?: ReactNode;
+    tooltip?: JSX.Element | string;


This is new to me - especially vs. ReactElement - would like to understand more

This change was to conform with Blueprint's accepted prop type (ReactNode was too broad)

cnrudd and others added 25 commits May 28, 2024 14:30

Restore support for ref prop in our components when written in JSX.

ed1529c

CHANGELOG.md

96c1e63

Reworked, much improved with Greg's help and review.

2010f89

Made hoist components limit ref prop only to those setup to handle it.

CHANGELOG.md update

1e7d3cf

Had missed fixing up desktop buttons.

d500557

Had forgotten to remove ref from ElementSpec.

Merge branch 'refs/heads/develop' into fixRefForJSX

d3f0025

# Conflicts: # CHANGELOG.md

Merge branch 'refs/heads/develop' into fixRefForJSX

a94d18f

# Conflicts: # CHANGELOG.md

Fix ref type on ButtonGroup.ts and ButtonGroupInput.ts

53b2506

Setup PinPadProps

9a7f3c8

Minor changes from CR with Lee and Greg.

722758d

Make ref argument passed to render generic + other ref-related ty…

99bd39a

…pe errors

Improve type handling for refs

68e6cf6

Overall type improvements and cleanup

18dee68

Fix RefreshButton docs

b8fe9df

Merge remote-tracking branch 'origin/develop' into fixRefForJSX

edc42f0

# Conflicts: # CHANGELOG.md # core/HoistComponent.ts # core/HoistProps.ts # desktop/cmp/input/ButtonGroupInput.ts

Fix HoistProps docstring

fb91079

Changes from CR w/ Lee

8feec54

Merge remote-tracking branch 'origin/develop' into fixRefForJSX

5ecbbb1

# Conflicts: # CHANGELOG.md # core/HoistComponent.ts

Minor cleanup

596aca9

Infer prop types for components passed to elementFactory

6182cd6

Fix NumberInput forwardRef bug

c7ab0db

Restore checkbox autoFocus functionality

aa18871

Minor cleanups / fixes

e7d8156

Merge remote-tracking branch 'origin/develop' into fixRefForJSX

853f04c

Revert added maxFiles prop to FileChooser

4a52025

ghsolomon mentioned this pull request Jun 28, 2024

Type improvements xh/toolbox#724

Open

Changes from CR

d99c671

amcclain reviewed Jun 28, 2024

View reviewed changes

Catch up with Develop

f4c587a

ghsolomon added 2 commits August 7, 2024 11:33

Remove obsolete ts-expect-error

bf4acb2

Fix CHANGELOG

e06da96

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overall type improvements #3707

Overall type improvements #3707

ghsolomon commented Jun 28, 2024 •

edited

Loading

amcclain Jun 28, 2024

amcclain left a comment

amcclain Jun 28, 2024

amcclain Jun 28, 2024

ghsolomon Jun 28, 2024

amcclain Jun 28, 2024

ghsolomon Jun 28, 2024

amcclain Jun 28, 2024

amcclain Jun 28, 2024

ghsolomon Jun 28, 2024

Overall type improvements #3707

Are you sure you want to change the base?

Overall type improvements #3707

Conversation

ghsolomon commented Jun 28, 2024 • edited Loading

Hoist P/R Checklist

Choose a reason for hiding this comment

amcclain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghsolomon commented Jun 28, 2024 •

edited

Loading