Skip to content
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

Overall type improvements #3707

Open
wants to merge 29 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
ed1529c
Restore support for ref prop in our components when written in JSX.
cnrudd May 28, 2024
96c1e63
CHANGELOG.md
cnrudd May 28, 2024
2010f89
Reworked, much improved with Greg's help and review.
cnrudd May 28, 2024
1e7d3cf
CHANGELOG.md update
cnrudd May 28, 2024
d500557
Had missed fixing up desktop buttons.
cnrudd May 29, 2024
d3f0025
Merge branch 'refs/heads/develop' into fixRefForJSX
cnrudd May 31, 2024
a94d18f
Merge branch 'refs/heads/develop' into fixRefForJSX
cnrudd Jun 3, 2024
53b2506
Fix ref type on ButtonGroup.ts and ButtonGroupInput.ts
cnrudd Jun 4, 2024
9a7f3c8
Setup PinPadProps
cnrudd Jun 4, 2024
722758d
Minor changes from CR with Lee and Greg.
cnrudd Jun 4, 2024
99bd39a
Make `ref` argument passed to `render` generic + other ref-related ty…
ghsolomon Jun 4, 2024
68e6cf6
Improve type handling for refs
ghsolomon Jun 5, 2024
18dee68
Overall type improvements and cleanup
ghsolomon Jun 7, 2024
b8fe9df
Fix RefreshButton docs
ghsolomon Jun 7, 2024
edc42f0
Merge remote-tracking branch 'origin/develop' into fixRefForJSX
ghsolomon Jun 7, 2024
fb91079
Fix HoistProps docstring
ghsolomon Jun 7, 2024
8feec54
Changes from CR w/ Lee
ghsolomon Jun 10, 2024
5ecbbb1
Merge remote-tracking branch 'origin/develop' into fixRefForJSX
ghsolomon Jun 25, 2024
596aca9
Minor cleanup
ghsolomon Jun 27, 2024
6182cd6
Infer prop types for components passed to elementFactory
ghsolomon Jun 27, 2024
c7ab0db
Fix NumberInput forwardRef bug
ghsolomon Jun 28, 2024
aa18871
Restore checkbox autoFocus functionality
ghsolomon Jun 28, 2024
e7d8156
Minor cleanups / fixes
ghsolomon Jun 28, 2024
853f04c
Merge remote-tracking branch 'origin/develop' into fixRefForJSX
ghsolomon Jun 28, 2024
4a52025
Revert added maxFiles prop to FileChooser
ghsolomon Jun 28, 2024
d99c671
Changes from CR
ghsolomon Jun 28, 2024
f4c587a
Catch up with Develop
ghsolomon Aug 7, 2024
bf4acb2
Remove obsolete ts-expect-error
ghsolomon Aug 7, 2024
e06da96
Fix CHANGELOG
ghsolomon Aug 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## 67.0.0-SNAPSHOT - unreleased

### 💥 Breaking Changes (upgrade difficulty: 🟢 TRIVIAL - renamed component prop)

* `RefreshButton` component `model` prop renamed to `target`.

### 🎁 New Features

* Added support for Correlation IDs across fetch requests and error / activity tracking:
Expand All @@ -23,10 +27,14 @@
### 🐞 Bug Fixes

* Fixed `SelectEditor` to ensure new value is flushed before editing stops.
* Fix bug where `model` passed to `RelativeTimestamp` was being ignored.

### ⚙️ Technical

* Remove context menus from column choosers.
* Typescript: Overall type improvements and cleanup. Note: `AppConfigs` with `model: false` will
need to specify a `null` model type in the generic argument to `hoistCmp`, `hoistCmp.factory` or
`hoistCmp.withFactory` to avoid a type error.

### 📚 Libraries

Expand Down
2 changes: 1 addition & 1 deletion admin/tabs/cluster/logs/LogViewerModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {LogDisplayModel} from './LogDisplayModel';
export class LogViewerModel extends BaseInstanceModel {
@observable file: string = null;

viewRef = createRef<HTMLElement>();
viewRef = createRef<HTMLDivElement>();

@managed
logDisplayModel = new LogDisplayModel(this);
Expand Down
2 changes: 1 addition & 1 deletion admin/tabs/cluster/websocket/WebSocketModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {RecordActionSpec} from '@xh/hoist/data';
import {AppModel} from '@xh/hoist/admin/AppModel';

export class WebSocketModel extends BaseInstanceModel {
viewRef = createRef<HTMLElement>();
viewRef = createRef<HTMLDivElement>();

@observable
lastRefresh: number;
Expand Down
2 changes: 1 addition & 1 deletion cmp/ag-grid/AgGrid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import './AgGrid.scss';
import {AgGridModel} from './AgGridModel';

export interface AgGridProps
extends HoistProps<AgGridModel>,
extends HoistProps<AgGridModel, HTMLDivElement>,
GridOptions,
LayoutProps,
TestSupportProps {}
Expand Down
4 changes: 2 additions & 2 deletions cmp/badge/Badge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
* Copyright © 2024 Extremely Heavy Industries Inc.
*/
import {div} from '@xh/hoist/cmp/layout';
import {BoxProps, hoistCmp, HoistProps, Intent} from '@xh/hoist/core';
import {BoxProps, hoistCmp, HoistPropsWithRef, Intent} from '@xh/hoist/core';
import {TEST_ID, mergeDeep} from '@xh/hoist/utils/js';
import {splitLayoutProps} from '@xh/hoist/utils/react';
import classNames from 'classnames';
import './Badge.scss';

export interface BadgeProps extends HoistProps, BoxProps {
export interface BadgeProps extends HoistPropsWithRef<HTMLDivElement>, BoxProps {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

/** Sets fontsize to half that of parent element (default false). */
compact?: boolean;

Expand Down
5 changes: 4 additions & 1 deletion cmp/chart/Chart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ import {LightTheme} from './theme/Light';
installZoomoutGesture(Highcharts);
installCopyToClipboard(Highcharts);

export interface ChartProps extends HoistProps<ChartModel>, LayoutProps, TestSupportProps {
export interface ChartProps
extends HoistProps<ChartModel, HTMLDivElement>,
LayoutProps,
TestSupportProps {
/**
* Ratio of width-to-height of displayed chart. If defined and greater than 0, the chart will
* respect this ratio within the available space. Otherwise, the chart will stretch on both
Expand Down
4 changes: 2 additions & 2 deletions cmp/clock/Clock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
BoxProps,
hoistCmp,
HoistModel,
HoistProps,
HoistPropsWithRef,
managed,
useLocalModel,
XH
Expand All @@ -21,7 +21,7 @@ import {MINUTES, ONE_SECOND} from '@xh/hoist/utils/datetime';
import {isNumber} from 'lodash';
import {getLayoutProps} from '../../utils/react';

export interface ClockProps extends HoistProps, BoxProps {
export interface ClockProps extends HoistPropsWithRef<HTMLDivElement>, BoxProps {
/** String to display if the timezone is invalid or an offset cannot be fetched. */
errorString?: string;

Expand Down
5 changes: 4 additions & 1 deletion cmp/dataview/DataView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@ import './DataView.scss';
import {DataViewModel} from './DataViewModel';
import {mergeDeep} from '@xh/hoist/utils/js';

export interface DataViewProps extends HoistProps<DataViewModel>, LayoutProps, TestSupportProps {
export interface DataViewProps
extends HoistProps<DataViewModel, HTMLDivElement>,
LayoutProps,
TestSupportProps {
/**
* Options for ag-Grid's API.
*
Expand Down
3 changes: 2 additions & 1 deletion cmp/filter/FilterChooserModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*
* Copyright © 2024 Extremely Heavy Industries Inc.
*/
import {HoistInputModel} from '@xh/hoist/cmp/input';
import {
HoistModel,
managed,
Expand Down Expand Up @@ -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>>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


constructor({
fieldSpecs,
Expand Down
2 changes: 1 addition & 1 deletion cmp/form/BaseFormFieldProps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {BoxProps, HoistProps} from '@xh/hoist/core';
import {ReactNode} from 'react';
import {FieldModel} from './field/FieldModel';

export interface BaseFormFieldProps extends HoistProps<FieldModel>, BoxProps {
export interface BaseFormFieldProps extends HoistProps<FieldModel, HTMLDivElement>, BoxProps {
/**
* CommitOnChange property for underlying HoistInput (for inputs that support).
* Defaulted from containing Form.
Expand Down
6 changes: 3 additions & 3 deletions cmp/form/Form.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
* Copyright © 2024 Extremely Heavy Industries Inc.
*/
import {
DefaultHoistProps,
elementFactory,
hoistCmp,
HoistProps,
PlainObject,
TestSupportProps,
uses
} from '@xh/hoist/core';
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/** Reference to associated FormModel. */
model?: FormModel;
Expand All @@ -44,7 +44,7 @@ export interface FormProps extends HoistProps<FormModel>, TestSupportProps {
* Defaults for certain props on child/nested FormFields.
* @see FormField (note there are both desktop and mobile implementations).
*/
fieldDefaults?: Partial<BaseFormFieldProps> & DefaultHoistProps;
fieldDefaults?: Partial<BaseFormFieldProps> & PlainObject;
}

/**
Expand Down
7 changes: 5 additions & 2 deletions cmp/grid/Grid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,10 @@ import {columnGroupHeader} from './impl/ColumnGroupHeader';
import {columnHeader} from './impl/ColumnHeader';
import {RowKeyNavSupport} from './impl/RowKeyNavSupport';

export interface GridProps extends HoistProps<GridModel>, LayoutProps, TestSupportProps {
export interface GridProps
extends HoistProps<GridModel, HTMLDivElement>,
LayoutProps,
TestSupportProps {
/**
* Options for ag-Grid's API.
*
Expand Down Expand Up @@ -145,7 +148,7 @@ export class GridLocalModel extends HoistModel {
@lookup(GridModel)
private model: GridModel;
agOptions: GridOptions;
viewRef = createObservableRef<HTMLElement>();
viewRef = createObservableRef<HTMLDivElement>();
private rowKeyNavSupport: RowKeyNavSupport;
private prevRs: RecordSet;

Expand Down
2 changes: 1 addition & 1 deletion cmp/grid/filter/GridFilterFieldSpec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export interface GridFilterFieldSpecConfig extends BaseFilterFieldSpecConfig {
* Props to pass through to the HoistInput components used on the custom filter tab.
* Note that the HoistInput component used is decided by fieldType.
*/
inputProps?: HoistInputProps;
inputProps?: HoistInputProps<unknown>;

/** Default operator displayed in custom filter tab. */
defaultOp?: FieldFilterOperator;
Expand Down
24 changes: 14 additions & 10 deletions cmp/grid/helpers/GridCountLabel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@
* Copyright © 2024 Extremely Heavy Industries Inc.
*/
import {box} from '@xh/hoist/cmp/layout';
import {BoxProps, hoistCmp, HoistProps, useContextModel} from '@xh/hoist/core';
import {BoxProps, hoistCmp, HoistPropsWithRef, useContextModel} from '@xh/hoist/core';
import {fmtNumber} from '@xh/hoist/format';
import {logError, pluralize, singularize, withDefault} from '@xh/hoist/utils/js';
import {GridModel} from '../GridModel';

export interface GridCountLabelProps extends HoistProps, BoxProps {
export interface GridCountLabelProps extends HoistPropsWithRef<HTMLDivElement>, BoxProps {
/** GridModel to which this component should bind. */
gridModel?: GridModel;

Expand Down Expand Up @@ -39,13 +39,16 @@ export const [GridCountLabel, gridCountLabel] = hoistCmp.withFactory<GridCountLa
displayName: 'GridCountLabel',
className: 'xh-grid-count-label',

render({
gridModel,
includeChildren = false,
showSelectionCount = 'auto',
unit = 'record',
...props
}) {
render(
{
gridModel,
includeChildren = false,
showSelectionCount = 'auto',
unit = 'record',
...props
},
ref
) {
gridModel = withDefault(gridModel, useContextModel(GridModel));

if (!gridModel) {
Expand Down Expand Up @@ -77,7 +80,8 @@ export const [GridCountLabel, gridCountLabel] = hoistCmp.withFactory<GridCountLa

return box({
...props,
item: `${recCountString()} ${selCountString()}`
item: `${recCountString()} ${selCountString()}`,
ref
});
}
});
19 changes: 11 additions & 8 deletions cmp/input/HoistInputModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* Copyright © 2024 Extremely Heavy Industries Inc.
*/
import {FieldModel} from '@xh/hoist/cmp/form';
import {DefaultHoistProps, HoistModel, HoistModelClass, useLocalModel} from '@xh/hoist/core';
import {HoistModel, HoistModelClass, PlainObject, useLocalModel} from '@xh/hoist/core';
import {action, computed, makeObservable, observable} from '@xh/hoist/mobx';
import {createObservableRef} from '@xh/hoist/utils/react';
import classNames from 'classnames';
Expand Down Expand Up @@ -51,10 +51,13 @@ import './HoistInput.scss';
* element of the rendered input via its `domEl` property, as well as `focus()`, `blur()`, and
* `select()`.
*
* @typeparam R - the type of HoistInputModel.inputRef (if any) - used to specify the type of the
* ref passed to the component.
*
* To create an instance of an Input component using this model use the hook
* {@link useHoistInputModel}.
*/
export class HoistInputModel extends HoistModel {
export class HoistInputModel<R> extends HoistModel {
/** Does this input have the focus? */
@observable hasFocus: boolean = false;

Expand Down Expand Up @@ -102,7 +105,7 @@ export class HoistInputModel extends HoistModel {
// Implementation State
//------------------------
@observable.ref internalValue: any = null; // Cached internal value
inputRef = createObservableRef<HTMLElement>(); // ref to internal <input> element, if any
inputRef = createObservableRef<R>(); // ref to internal <input> element, if any
domRef = createObservableRef<HTMLElement>(); // ref to outermost element, or class Component.
isDirty: boolean = false;

Expand Down Expand Up @@ -326,13 +329,13 @@ export class HoistInputModel extends HoistModel {
* @param ref - forwardRef passed to containing component
* @param modelSpec - specify to use particular subclass of HoistInputModel
*/
export function useHoistInputModel(
export function useHoistInputModel<R>(
component: any,
props: DefaultHoistProps,
ref: ForwardedRef<any>,
modelSpec?: HoistModelClass<HoistInputModel>
props: PlainObject,
ref: ForwardedRef<HoistInputModel<R>>,
modelSpec?: HoistModelClass<HoistInputModel<R>>
): ReactElement {
const inputModel = useLocalModel<HoistInputModel>(modelSpec ?? HoistInputModel);
const inputModel = useLocalModel<HoistInputModel<R>>(modelSpec ?? HoistInputModel);

useImperativeHandle(ref, () => inputModel);

Expand Down
15 changes: 13 additions & 2 deletions cmp/input/HoistInputProps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,17 @@
* Copyright © 2024 Extremely Heavy Industries Inc.
*/

import {TestSupportProps} from '@xh/hoist/core';
import {HoistInputModel} from '@xh/hoist/cmp/input/HoistInputModel';
import {HoistModel, HoistProps, TestSupportProps} from '@xh/hoist/core';
import {CSSProperties} from 'react';

export interface HoistInputProps extends TestSupportProps {
/**
* Props for HoistInput components.
* @typeparam R - the type of HoistInputModel.inputRef (if any) for this component.
*/
export interface HoistInputProps<R>
extends TestSupportProps,
HoistProps<HoistModel, HoistInputModel<R>> {
/**
* Field or model property name from which this component should read and write its value
* in controlled mode. Can be set by parent FormField.
Expand All @@ -26,6 +34,9 @@ export interface HoistInputProps extends TestSupportProps {
/** Called when value is committed to backing model - passed new and prior values. */
onCommit?: (value: any, oldValue: any) => void;

/** CSS style attributes for the input element. */
style?: CSSProperties;

/** Tab order for focus control, or -1 to skip. If unset, browser layout-based order. */
tabIndex?: number;

Expand Down
4 changes: 2 additions & 2 deletions cmp/layout/Box.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@
*
* Copyright © 2024 Extremely Heavy Industries Inc.
*/
import {BoxProps, hoistCmp, HoistProps} from '@xh/hoist/core';
import {BoxProps, hoistCmp, HoistPropsWithRef} from '@xh/hoist/core';
import {TEST_ID, mergeDeep} from '@xh/hoist/utils/js';
import {splitLayoutProps} from '@xh/hoist/utils/react';
import {div} from './Tags';

export interface BoxComponentProps extends HoistProps, BoxProps {}
export interface BoxComponentProps extends HoistPropsWithRef<HTMLDivElement>, BoxProps {}

/**
* A Component that supports flexbox-based layout of its contents.
Expand Down
4 changes: 2 additions & 2 deletions cmp/layout/Frame.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
*
* Copyright © 2024 Extremely Heavy Industries Inc.
*/
import {hoistCmp, BoxProps, HoistProps} from '@xh/hoist/core';
import {hoistCmp, BoxProps, HoistPropsWithRef} from '@xh/hoist/core';
import {box} from './Box';

export interface FrameProps extends HoistProps, BoxProps {}
export interface FrameProps extends HoistPropsWithRef<HTMLDivElement>, BoxProps {}

/**
* A Box class that flexes to grow and stretch within its *own* parent via flex:'auto', useful for
Expand Down
4 changes: 2 additions & 2 deletions cmp/layout/Placeholder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
*
* Copyright © 2024 Extremely Heavy Industries Inc.
*/
import {hoistCmp, BoxProps, setCmpErrorDisplay, HoistProps} from '@xh/hoist/core';
import {hoistCmp, BoxProps, setCmpErrorDisplay, HoistPropsWithRef} from '@xh/hoist/core';
import {box} from '@xh/hoist/cmp/layout';
import './Placeholder.scss';

export interface PlaceholderProps extends HoistProps, BoxProps {}
export interface PlaceholderProps extends HoistPropsWithRef<HTMLDivElement>, BoxProps {}

/**
* A thin wrapper around `Box` with standardized, muted styling.
Expand Down
6 changes: 3 additions & 3 deletions cmp/layout/Spacer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
*
* Copyright © 2024 Extremely Heavy Industries Inc.
*/
import {hoistCmp, BoxProps, HoistProps} from '@xh/hoist/core';
import {hoistCmp, BoxProps, HoistPropsWithRef, HoistProps} from '@xh/hoist/core';
import {box} from './Box';

export interface SpacerProps extends HoistProps, BoxProps {}
export interface SpacerProps extends HoistPropsWithRef<HTMLDivElement>, BoxProps {}

/**
* A component for inserting a fixed-sized spacer along the main axis of its parent container.
Expand All @@ -31,7 +31,7 @@ export const [Spacer, spacer] = hoistCmp.withFactory<SpacerProps>({
/**
* A component that stretches to soak up space along the main axis of its parent container.
*/
export const [Filler, filler] = hoistCmp.withFactory<BoxProps>({
export const [Filler, filler] = hoistCmp.withFactory<BoxProps & HoistProps>({
displayName: 'Filler',
model: false,
observer: false,
Expand Down
Loading
Loading