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.

Sign up for GitHub

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

Jump to bottom

Date picker component #1499

Merged
merged 4 commits into from
Dec 30, 2022
Merged

Date picker component #1499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f9d3367
Merge branch 'master' into date-picker-component
bytasv Dec 27, 2022
a368ada
Add datepicker
bytasv Dec 29, 2022
6a1de04
Merge branch 'master' into date-picker-component
bytasv Dec 29, 2022
19b11c6
Fix typing
bytasv Dec 30, 2022
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
15 changes: 13 additions & 2 deletions packages/toolpad-app/src/runtime/ToolpadApp.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,12 +266,23 @@ function RenderedNodeContent({ node, childNodeGroups, Component }: RenderedNodeC

const handler = (param: any) => {
const bindingId = `${nodeId}.props.${key}`;
const value = argType.onChangeHandler ? argType.onChangeHandler(param) : param;

const defaultValues = _.mapValues(argTypes, ({ defaultValue }: any) => defaultValue);
const propsValues = _.mapValues(
(node as appDom.ElementNode).props,
(prop) => prop?.value || undefined,
);
const props = {
...defaultValues,
...propsValues,
};

const value = argType.onChangeHandler ? argType.onChangeHandler(param, props) : param;
setControlledBinding(bindingId, { value });
};
return [argType.onChangeProp, handler];
}),
[argTypes, nodeId, setControlledBinding],
[argTypes, nodeId, setControlledBinding, node],
);

const navigateToPage = usePageNavigator();
Expand Down
6 changes: 4 additions & 2 deletions packages/toolpad-app/src/toolpad/AppEditor/PageEditor/BindableEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,13 @@ export default function BindableEditor<V>({
);

const initConstValue = React.useCallback(() => {
let constValue = liveBinding?.value;

if (value?.type === 'const') {
return value.value;
constValue = value.value;
}

return liveBinding?.value;
return constValue;
}, [liveBinding, value]);

const constValue = React.useMemo(initConstValue, [value, initConstValue]);
Expand Down
34 changes: 14 additions & 20 deletions packages/toolpad-app/src/toolpad/AppEditor/PageEditor/ComponentEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,26 +90,20 @@ function ComponentPropsEditor<P extends object>({
<Typography variant="overline" className={classes.sectionHeading}>
Layout:
</Typography>
{hasLayoutHorizontalControls ? (
<div className={classes.control}>
<NodeAttributeEditor
node={node}
namespace="layout"
name="horizontalAlign"
argType={layoutBoxArgTypes.horizontalAlign}
/>
</div>
) : null}
{hasLayoutVerticalControls ? (
<div className={classes.control}>
<NodeAttributeEditor
node={node}
namespace="layout"
name="verticalAlign"
argType={layoutBoxArgTypes.verticalAlign}
/>
</div>
) : null}

<div className={classes.control}>
<NodeAttributeEditor
node={node}
namespace="layout"
name={hasLayoutHorizontalControls ? 'horizontalAlign' : 'verticalAlign'}
argType={
hasLayoutHorizontalControls
? layoutBoxArgTypes.horizontalAlign
: layoutBoxArgTypes.verticalAlign
}
/>
</div>

<Divider sx={{ mt: 1 }} />
</React.Fragment>
) : null}
Expand Down
1 change: 1 addition & 0 deletions packages/toolpad-app/src/toolpadComponents/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const INTERNAL_COMPONENTS = new Map<string, ToolpadComponentDefinition>([
['Image', { displayName: 'Image', builtIn: 'Image' }],
['DataGrid', { displayName: 'Data grid', builtIn: 'DataGrid' }],
['TextField', { displayName: 'Text field', builtIn: 'TextField' }],
['DatePicker', { displayName: 'Date picker', builtIn: 'DatePicker' }],
['Text', { displayName: 'Text', builtIn: 'Text' }],
['Select', { displayName: 'Select', builtIn: 'Select' }],
['Paper', { displayName: 'Paper', builtIn: 'Paper' }],
Expand Down
93 changes: 93 additions & 0 deletions packages/toolpad-components/src/DatePicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import * as React from 'react';

import { TextField } from '@mui/material';

import { LocalizationProvider } from '@mui/x-date-pickers/LocalizationProvider';
import { DesktopDatePicker, DesktopDatePickerProps } from '@mui/x-date-pickers/DesktopDatePicker';
import { AdapterDayjs } from '@mui/x-date-pickers/AdapterDayjs';
import { createComponent } from '@mui/toolpad-core';
import { Dayjs } from 'dayjs';

export interface Props extends DesktopDatePickerProps<string, Dayjs> {
format: string;
separator: string;
fullWidth: boolean;
variant: 'outlined' | 'filled' | 'standard';
size: 'small' | 'medium';
sx: any;
}

const resolveFormat = (format: string, separator: string) => format.replaceAll(' ', separator);

function DatePicker(props: Props) {
return (
<LocalizationProvider dateAdapter={AdapterDayjs as any}>
<DesktopDatePicker
inputFormat={resolveFormat(props.format, props.separator)}
{...props}
renderInput={(params) => (
<TextField
{...params}
fullWidth={props.fullWidth}
variant={props.variant}
size={props.size}
sx={props.sx}
/>
)}
/>
</LocalizationProvider>
);
}

export default createComponent(DatePicker, {
argTypes: {
value: {
typeDef: { type: 'string' },
onChangeProp: 'onChange',
onChangeHandler: (newValue: Dayjs, { format, separator }: Props) => {
Copy link
Member

Choose a reason for hiding this comment

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

For uniformity, I prefer to keep the convention of

value: T
onChange: (newValue: T) => void

I prefer to not ad hoc update the signature of onChangeHandler until we have more compelling use-cases to do so.

I think we should standardize on toISOString() for the format of value and newValue, so that the developer doesn't have to consider the formatting when handling this date.

for a benchmark, refer to retool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For uniformity, I prefer to keep the convention of
Do you consider accessing props in a change handler is something we would never need? Even though this seems like a good example where this is needed and makes sense

  1. Is there a different way to access props in change handler if I wanted to know which format is selected?
  2. If 1st is a NO then that means that we are getting rid of format prop?

Copy link
Member

@Janpot Janpot Jan 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  1. NO
  2. We don't have ti use the onChangeHandler. We can implement the logic in the component itself, where we have access to the live property values. Similar to how we do onSelectionChange in the DataGrid

regarding onChangeHandler, this is a remnant from a time when we were still very optimistic we would be able build the components library by just taking all the bare MUI components and wrapping each of them with createComponent. In the meantime it has become clear that we needed more customization on most components. Or components that don't exist in MUI at all. And so the onChangeHandler has become a bit obsolete. Maybe we should even think about getting rid of it altogether

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can implement the logic in the component itself, where we have access to the live property values

I had that initially done, however using value of datePicker in other places resulted in raw iso string which seemed wrong to me. But sure we can do that

Copy link
Member

Choose a reason for hiding this comment

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

I think we can use the date-only form of ISO8601. This is also unambiguously usable as input for new Date()

return newValue.format(resolveFormat(format, separator));
},
defaultValue: '',
defaultValueProp: 'defaultValue',
},
format: {
Copy link
Member

Choose a reason for hiding this comment

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

Currently, the Toolpad app developer decides for every user which format they have to input dates in. I wonder if it rather makes more sense that users input the date in the format based on their locale

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wonder if it rather makes more sense that users input the date in the format based on their locale

I'm not really sure how that would work if say 2 users with different locales edited app, which one should be used? Or is there a possibility that DD-MM-YYYY on one machine might get converted to MM-DD-YYYY on the other? Also what if I don't want to use my locale because I'm building an app for users in a different locale?
IMO having an option to choose custom format should be among something that user is able to control, even retool has it:
CleanShot 2023-01-02 at 11 54 07@2x

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 not really sure how that would work if say 2 users with different locales edited app, which one should be used?

Each user in the locale of their browser. The value prop should be kept in an unambiguous format so that it doesn't matter to the runtime.

Also what if I don't want to use my locale because I'm building an app for users in a different locale?

Those end users will have their browsers set to their own locale, so if the date input just follows that, then it should be fine. What if I build applications for users in mixed locales? e.g. like in MUI: I, in Belgium should see "31/12/2022", and someone in US should see 12-31-2022, and the value prop should be 2022-12-31 for both cases

IMO having an option to choose custom format should be among something that user is able to control

Which user, the app editor? or the end user? in any case, we can always do both options, default to "use end user locale" and allow overriding with a custom format (and we can probably consolidate the separator and format props into 1 "format" prop).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value prop should be kept in an unambiguous format so that it doesn't matter to the runtime.

When I use datePicker.value in a text field somewhere do we display raw value or formatted with locale? If formatted - do we check all values if it matches date string or shall we also use some meta data from datepicker field to know that we are dealing with date value?

in any case, we can always do both options, default to "use end user locale" and allow overriding with a custom format

Do I understand correctly that you'd like to have format input field which is free form text field allowing to pass any format that is supported and if nothing is provided default to locale format?

Copy link
Member

Choose a reason for hiding this comment

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

When I use datePicker.value in a text field somewhere do we display raw value or formatted with locale?

I believe we should keep this value unformatted. i.e. a shorthand version of ISO8601 should be fine (2022-12-31). I think the most important is that this value is unambiguous, I should be able to do new Date(datePicker.value) and that should interpret the correct date everywhere. This is important because often these values will be sent back to the server, and will then lose all the context of the end user locale.
If the user wants to display such a value somewhere in the UI, for now they'll have to expose a format function on the global scope and use that to display the date. Fully aware this has more friction than it could have, but we can iterate on that in a separate effort.

Do I understand correctly that you'd like to have format input field which is free form text field allowing to pass any format that is supported and if nothing is provided default to locale format?

It's an option yes, my preference would be free form text, leave empty to use end user locale, default empty. But we can iterate on the idea.

typeDef: {
type: 'string',
enum: ['DD MM YYYY', 'YYYY MM DD', 'MM DD YYYY'],
},
defaultValue: 'YYYY MM DD',
},
separator: {
typeDef: {
type: 'string',
enum: ['-', '/', ' '],
},
defaultValue: '-',
},
// @ts-ignore no idea why it complains even though it's done exactly same as TextField
defaultValue: {
typeDef: { type: 'string' },
defaultValue: '',
},
label: {
typeDef: { type: 'string' },
},
variant: {
typeDef: { type: 'string', enum: ['outlined', 'filled', 'standard'] },
defaultValue: 'outlined',
},
size: {
typeDef: { type: 'string', enum: ['small', 'medium'] },
defaultValue: 'small',
},
fullWidth: {
typeDef: { type: 'boolean' },
},
disabled: {
typeDef: { type: 'boolean' },
},
sx: {
typeDef: { type: 'object' },
},
},
});
2 changes: 2 additions & 0 deletions packages/toolpad-components/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,7 @@ export { default as Paper } from './Paper.js';

export { default as Image } from './Image.js';

export { default as DatePicker } from './DatePicker.js';

export { CUSTOM_COLUMN_TYPES, NUMBER_FORMAT_PRESETS, inferColumns, parseColumns } from './DataGrid';
export type { SerializableGridColumn, SerializableGridColumns, NumberFormat } from './DataGrid';