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

Improve onError validation #1730

Merged
merged 27 commits into from
May 6, 2020
Merged

Improve onError validation #1730

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
5563142
Introduce reason-based validation
dmtrKovalenko Apr 30, 2020
70b66d4
Remove dead validation code
dmtrKovalenko Apr 30, 2020
e9a0687
Implement new validation for TimePicker and DateTimePicker
dmtrKovalenko Apr 30, 2020
4f2207f
Remove redux form docs example
dmtrKovalenko Apr 30, 2020
868358a
Add Formik with validation schema example
dmtrKovalenko Apr 30, 2020
679ea8d
More tests
dmtrKovalenko Apr 30, 2020
a501984
TimePicker validation tests
dmtrKovalenko Apr 30, 2020
a1a6b42
Move parsing min/max date up to the root component
dmtrKovalenko May 1, 2020
5ca8ef2
Use touched state in the formik example
dmtrKovalenko May 1, 2020
3af9888
Remove console.logs
dmtrKovalenko May 1, 2020
71981e0
Merge conflicts
dmtrKovalenko May 1, 2020
cbe5fc4
Fix lint and build errors
dmtrKovalenko May 1, 2020
3eddd8b
Remove visual regression flakiness with time validation
dmtrKovalenko May 1, 2020
f21609c
Remove emptyInputText
dmtrKovalenko May 1, 2020
754a95b
Fix validation tests
dmtrKovalenko May 1, 2020
58f12c7
Commit .size-snapshot
dmtrKovalenko May 1, 2020
e349496
Implement validation for DateRangePicker.tsx
dmtrKovalenko May 1, 2020
5bce1e4
Add DateRange validation tests
dmtrKovalenko May 1, 2020
e4166a7
Fix linter
dmtrKovalenko May 1, 2020
860d6f5
Fix broken design of date rangepicker input parsing
dmtrKovalenko May 5, 2020
c73d4e7
Merge conflicts
dmtrKovalenko May 5, 2020
8dedc04
Remove <Code> from formik examples
dmtrKovalenko May 5, 2020
66eb022
Update yarn.lock
dmtrKovalenko May 5, 2020
af381ef
Update docs/pages/guides/FormikOurValidation.example.tsx
dmtrKovalenko May 5, 2020
1201eef
Update docs/pages/guides/FormikOurValidation.example.tsx
dmtrKovalenko May 5, 2020
79cea16
Update docs/pages/guides/FormikOurValidation.example.tsx
dmtrKovalenko May 5, 2020
806b63c
Update new forms example to be more consolidated with @materail-ui/core
dmtrKovalenko May 6, 2020
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
Prev Previous commit
Next Next commit
Implement validation for DateRangePicker.tsx
  • Loading branch information
@dmtrKovalenko
dmtrKovalenko committed May 1, 2020
commit e349496507a48e71bddc1934ba3171af120f84d0
2 changes: 1 addition & 1 deletion docs/pages/guides/FormikOurValidation.example.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ const DatePickerField: React.FC<DatePickerFieldProps> = ({

const FormikExample = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const FormikExample = () => {
export default FormikOurValidation() {

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer export default to be always the last export. It is more readable.

Copy link
Member

Choose a reason for hiding this comment

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

We systematically use the other way around to 1. increase the code density of the demos, 2. reduce the duplicate of the name of the demo. I think that it's important we still to a single convention.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but for now pickers are standalone project. And if we are not planning to migrate docs before v5 we could stick to the our internal conventions for now. (before we merge docs)

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 that we can break down the dicussion in 3 different topics:

  1. Standaloneness. The picker components are part of Material-UI, like any other official component. I think that we should treat the component here as they were inside the mono repository. Where the code is a hosted shouldn't influence the developer experience. I think that it should be to the own discretion of how the core team wants to work.
    Why? The consistency of the components is one of the important value propositions of the library, I think that steps in this direction improve the DX.
  2. Progressiveness. I think that we should leverage all the opportunities to reduce the gap in the "standard way". I think that any step in the long term direction is good to take. What the advantage of delaying it? I find it interesting to handle it as we go, as it will be one less change to handle in the future.
  3. Convention evolution. I think that we should evolve the convention as we learn from our users and the environment change. RFC sounds like the best framework to make changes.
    For history, we used to follow the approach of the pull request, it was optimized for reducing the switching cost between class and functional components. With hooks, this requirement is no longer relevant.

return (
<Formik onSubmit={alert} initialValues={{ date: new Date() }}>
<Formik onSubmit={console.log} initialValues={{ date: new Date() }}>
dmtrKovalenko marked this conversation as resolved.
Show resolved Hide resolved
{({ values, errors }) => (
<Form>
<Grid container>
Expand Down
2 changes: 1 addition & 1 deletion docs/pages/guides/FormikValidationSchema.example.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ const schema = object({

const FormikExample = () => {
return (
<Formik validationSchema={schema} onSubmit={alert} initialValues={{ date: new Date() }}>
<Formik validationSchema={schema} onSubmit={console.log} initialValues={{ date: new Date() }}>
{({ values, errors }) => (
<Form>
<Grid container>
Expand Down
101 changes: 57 additions & 44 deletions docs/prop-types.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

76 changes: 34 additions & 42 deletions lib/src/DateRangePicker/DateRangePicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,31 @@
import * as React from 'react';
import { MaterialUiPickersDate } from '../typings/date';
import { BasePickerProps } from '../typings/BasePicker';
import { useUtils } from '../_shared/hooks/useUtils';
import { MobileWrapper } from '../wrappers/MobileWrapper';
import { DateRangeInputProps } from './DateRangePickerInput';
import { parsePickerInputValue } from '../_helpers/date-utils';
import { usePickerState } from '../_shared/hooks/usePickerState';
import { useParsedDate } from '../_shared/hooks/date-helpers-hooks';
import { DesktopPopperWrapper } from '../wrappers/DesktopPopperWrapper';
import { MuiPickersAdapter, useUtils } from '../_shared/hooks/useUtils';
import { makeWrapperComponent } from '../wrappers/makeWrapperComponent';
import { ResponsivePopperWrapper } from '../wrappers/ResponsiveWrapper';
import { defaultMinDate, defaultMaxDate } from '../constants/prop-types';
import { SomeWrapper, ExtendWrapper, StaticWrapper } from '../wrappers/Wrapper';
import { makeValidationHook, ValidationProps } from '../_shared/hooks/useValidation';
import { DateRangePickerView, ExportedDateRangePickerViewProps } from './DateRangePickerView';
import { DateRangePickerInput, ExportedDateRangePickerInputProps } from './DateRangePickerInput';
import {
DateRange as DateRangeType,
RangeInput,
AllSharedDateRangePickerProps,
} from './RangeTypes';

export function parseRangeInputValue(
now: MaterialUiPickersDate,
utils: MuiPickersAdapter,
{ value = [null, null], defaultHighlight }: BasePickerProps<RangeInput, DateRange>
) {
return value.map(date =>
date === null
? null
: utils.startOfDay(parsePickerInputValue(now, utils, { value: date, defaultHighlight }))
) as DateRangeType;
}
import {
parseRangeInputValue,
validateDateRange,
DateRangeValidationError,
} from '../_helpers/date-utils';

export interface DateRangePickerProps
extends ExportedDateRangePickerViewProps,
ValidationProps<DateRangeValidationError, DateRangeType>,
ExportedDateRangePickerInputProps {
/**
* Text for start input label and toolbar placeholder
Expand All @@ -45,6 +39,12 @@ export interface DateRangePickerProps
endText?: React.ReactNode;
}

export const useDateRangeValidation = makeValidationHook<
DateRangeValidationError,
DateRangeType,
DateRangePickerProps
>(validateDateRange);

export function makeRangePicker<TWrapper extends SomeWrapper>(Wrapper: TWrapper) {
const WrapperComponent = makeWrapperComponent<DateRangeInputProps, RangeInput, DateRange>(
Wrapper,
Expand All @@ -56,35 +56,36 @@ export function makeRangePicker<TWrapper extends SomeWrapper>(Wrapper: TWrapper)

function RangePickerWithStateAndWrapper({
calendars,
minDate,
maxDate,
disablePast,
disableFuture,
shouldDisableDate,
showDaysOutsideCurrentMonth,
onMonthChange,
disableHighlightToday,
reduceAnimations,
value,
onChange,
mask = '__/__/____',
startText = 'Start',
endText = 'End',
inputFormat: passedInputFormat,
...restPropsForTextField
minDate: __minDate = defaultMinDate,
Copy link
Member

Choose a reason for hiding this comment

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

For consistency

Suggested change
minDate: __minDate = defaultMinDate,
minDate: minDateProp = defaultMinDate,

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't it look less clear? Because accidentally type minDateProp when you are typing minDate is much much easier than __minDate. And __ is some kind of convention about naming private variables?

Copy link
Member

@oliviertassinari oliviertassinari May 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

No specific preference, we could make it the new convention, if we do, could you update all the other components (core, lab, etc.)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably yes :)
Ill give you more context: Here we had exactly the same naming minDatePropbut it was problematic because appeared in the autocomplete forminDate` so I decided to use another convention

maxDate: __maxDate = defaultMaxDate,
...other
}: DateRangePickerProps & AllSharedDateRangePickerProps & ExtendWrapper<TWrapper>) {
const utils = useUtils();
const minDate = useParsedDate(__minDate);
const maxDate = useParsedDate(__maxDate);
const [currentlySelectingRangeEnd, setCurrentlySelectingRangeEnd] = React.useState<
'start' | 'end'
>('start');

const pickerStateProps = {
...restPropsForTextField,
...other,
value,
onChange,
inputFormat: passedInputFormat || utils.formats.keyboardDate,
};

const restProps = {
...other,
minDate,
maxDate,
};

const { pickerProps, inputProps, wrapperProps } = usePickerState<RangeInput, DateRange>(
pickerStateProps,
{
Expand All @@ -94,40 +95,31 @@ export function makeRangePicker<TWrapper extends SomeWrapper>(Wrapper: TWrapper)
}
);

const validationError = useDateRangeValidation(pickerProps.date, restProps);

const DateInputProps = {
...inputProps,
...restPropsForTextField,
...restProps,
currentlySelectingRangeEnd,
setCurrentlySelectingRangeEnd,
startText,
endText,
mask,
validationError,
};

return (
<WrapperComponent
wrapperProps={wrapperProps}
DateInputProps={DateInputProps}
{...restPropsForTextField}
>
<WrapperComponent wrapperProps={wrapperProps} DateInputProps={DateInputProps} {...restProps}>
<DateRangePickerView
open={wrapperProps.open}
DateInputProps={DateInputProps}
calendars={calendars}
minDate={minDate}
maxDate={maxDate}
disablePast={disablePast}
disableFuture={disableFuture}
shouldDisableDate={shouldDisableDate}
showDaysOutsideCurrentMonth={showDaysOutsideCurrentMonth}
onMonthChange={onMonthChange}
disableHighlightToday={disableHighlightToday}
reduceAnimations={reduceAnimations}
currentlySelectingRangeEnd={currentlySelectingRangeEnd}
setCurrentlySelectingRangeEnd={setCurrentlySelectingRangeEnd}
startText={startText}
endText={endText}
{...pickerProps}
{...restProps}
/>
</WrapperComponent>
);
Expand Down
Loading