Conversation
![accesslint[bot]](https://avatars.githubusercontent.com/in/1502?s=60&v=4){kind=small}
| ] | ||
| } | ||
| selectedOption={9} | ||
| /> |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| ] | ||
| } | ||
| selectedOption="December" | ||
| /> |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
|
|
||
| const DaySelector = ( | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
|
|
||
| const MonthSelector = ( | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
|
|
||
| const YearSelector = ( | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| ] | ||
| } | ||
| selectedOption={2019} | ||
| /> |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
|
|
||
| const MonthSelector = ( | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
|
|
||
| const YearSelector = ( | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
|
|
||
| const MonthSelector = ( | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
|
|
||
| const YearSelector = ( | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| <> | ||
| <label htmlFor="date" style={{display: 'none'}}>Date</label> | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| Month | ||
| </label> | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| <> | ||
| <label htmlFor="year" style={{display: 'none'}}>Year</label> | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| id: string, | ||
| className?: string, | ||
| name: string, | ||
| name?: string, |
There was a problem hiding this comment.
Name should be mandatory
| {placeholder && <option value="">{placeholder}</option>} | ||
| {options.map(opt => ( | ||
| <option key={opt} value={opt}> | ||
| <option key={`${name}_${opt}`} value={opt}> |
There was a problem hiding this comment.
Nice, instead of ${opt} use index that will avoid white space
| import Select from '../../atoms/Select'; | ||
| import Label from '../../atoms/Label'; | ||
|
|
||
| const monthOptions = [ |
There was a problem hiding this comment.
How will these change if locale changes to lets say French or Spanish
There was a problem hiding this comment.
I think we can get this dynamically, from browser itself and then we will no longer need to take care of locales
| ]; | ||
|
|
||
| const DateSelector = (props: Props): Node => { | ||
| const { className, format, startDate, endDate } = props; |
There was a problem hiding this comment.
Move this line at like 31
(props: Props) --> { className, format, startDate, endDate } will achieve same thing
| const DateSelector = (props: Props): Node => { | ||
| const { className, format, startDate, endDate } = props; | ||
| const [selectedDate, setSelectedDate] = useState(new Date()); | ||
| const [newStartDate, setNewStartDate] = useState( |
There was a problem hiding this comment.
How is startDate and newStartDate diff?
There was a problem hiding this comment.
removed the startDate from state
| new Date(new Date().setFullYear(new Date().getFullYear() - 100)) | ||
| ); | ||
| const [newEndDate, setNewEndDate] = useState( | ||
| new Date(new Date().setFullYear(new Date().getFullYear() + 50)) |
There was a problem hiding this comment.
Where did you find this 50 or 100 number?
There was a problem hiding this comment.
Those were for the default start and end date
vinodloha
left a comment
vinodloha
left a comment
There was a problem hiding this comment.
Looks like a good progress but still far way from being complete
| setNewStartDate(new Date(startDate)); | ||
| } else { | ||
| const d = new Date(); | ||
| setNewStartDate(new Date(d.setFullYear(d.getFullYear() - 100))); |
There was a problem hiding this comment.
new Date(d.setFullYear(d.getFullYear() - 100) get repeated too often, create a method for it
| ); | ||
|
|
||
| const setDate = dateType => { | ||
| if (dateType === 'start') { |
There was a problem hiding this comment.
Such else and If cases produce duplicate code, the same function should be able to handle or set start or end date
the old diff is + and - that can be managed with a small ? : ;
There was a problem hiding this comment.
Changed the logic
| const days = []; | ||
| let startDay = 1; | ||
| let endDay = noOfDays; | ||
| if ( |
There was a problem hiding this comment.
I don't understand what this logic below is doing, first line itself has given you all the values.
You must break this complexity
There was a problem hiding this comment.
That was a mistake. Made changes
| ) { | ||
| endDay = newEndDate.getDate(); | ||
| } | ||
| for (let i = startDay; i <= endDay; i += 1) { |
There was a problem hiding this comment.
Hope you know end date and start date are optional features so that might be required or might not be in some use-cases
There was a problem hiding this comment.
these are optional features but to show default start and end dates I'm using these
|
|
||
| const getMonthOptions = () => { | ||
| let months = [...monthOptions]; | ||
| if (selectedDate.getFullYear() === newStartDate.getFullYear()) { |
| }; | ||
|
|
||
| const DaySelector = ( | ||
| <Label htmlFor="day"> |
There was a problem hiding this comment.
Where is text for label? don't use the label to wrap the field set relation ship with for htmlFor="{${id}_day}"
| new Date(selectedDate.getFullYear(), selectedDate.getMonth(), e.target.value) | ||
| ); | ||
| }} | ||
| id="day" |
There was a problem hiding this comment.
Don't hard code id, create one from the one passed for whole component
| id="day" | ||
| name="day" | ||
| className={classNames('date', className)} | ||
| disabled={false} |
There was a problem hiding this comment.
disabled={false} ??? hard coding
|
|
||
| return ( | ||
| <div className={className}> | ||
| {(format.toLowerCase() === 'ddmmyy' || format.toLowerCase() === 'ddmmyyyy') && [ |
| children: Node, | ||
| className?: string, | ||
| format?: Format, | ||
| startDate: number, |
There was a problem hiding this comment.
startDate and endDate are also optional
vinodloha
added
invalid
| placeholder: '', | ||
| className: '', | ||
| elementLocator: '', | ||
| name: 'optionName', |
There was a problem hiding this comment.
Name should not be part of default props also don't like how we are setting empty default values, please fix it too
vinodloha
left a comment
vinodloha
left a comment
There was a problem hiding this comment.
Please work on review comments
vinodloha
changed the base branch from
feature/design-system-basics
to
develop
| ] | ||
| } | ||
| selectedOption={16} | ||
| /> |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| name="month" | ||
| onChange={[Function]} | ||
| options={Array []} | ||
| /> |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| <> | ||
| <Label htmlFor={`${id}-day`} /> | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| <> | ||
| <Label htmlFor={`${id}-month`} /> | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| ] | ||
| } | ||
| selectedOption={2019} | ||
| /> |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| <> | ||
| <Label htmlFor={`${id}-year`} /> | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| ] | ||
| } | ||
| selectedOption={17} | ||
| /> |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| name="month" | ||
| onChange={[Function]} | ||
| options={Array []} | ||
| /> |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| ] | ||
| } | ||
| selectedOption={2019} | ||
| /> |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| ] | ||
| } | ||
| selectedOption={17} | ||
| /> |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| name="month" | ||
| onChange={[Function]} | ||
| options={Array []} | ||
| /> |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| <> | ||
| <Label htmlFor={`${id}-day`}>{dateLabel}</Label> | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| <> | ||
| <Label htmlFor={`${id}-month`}>{monthLabel}</Label> | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| ] | ||
| } | ||
| selectedOption={2019} | ||
| /> |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| <> | ||
| <Label htmlFor={`${id}-year`}>{yearLabel}</Label> | ||
| <Select | ||
| onChange={e => { |
There was a problem hiding this comment.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
| placeholder: '', | ||
| className: '', | ||
| elementLocator: '', | ||
| selectedOption: 'select', |
There was a problem hiding this comment.
By default selected option should be null
| selectedOption: 'select', | ||
| placeholder: 'Select', | ||
| className: 'select', | ||
| elementLocator: 'select', |
There was a problem hiding this comment.
Set elementLocator default value as null
| ) { | ||
| startDay = startDate.getDate(); | ||
| } | ||
| if ( |
There was a problem hiding this comment.
Still there is redundant code we can reduce, for example:
- These 2 If conditions were we are startDay and endDay could be returned by a function.
|
|
||
| const getMonthOptions = () => { | ||
| let monthOptions = [...months]; | ||
| if (selectedDate.getFullYear() === startDate.getFullYear()) { |
There was a problem hiding this comment.
Same here, this could be returned by a function and also clear case of If or else.
|
|
||
| const getYearOptions = () => { | ||
| const years = []; | ||
| for (let i = startDate.getFullYear(); i <= endDate.getFullYear(); i += 1) { |
There was a problem hiding this comment.
| ); | ||
|
|
||
| const renderSelector = value => { | ||
| switch (value.toLowerCase()) { |
| @@ -0,0 +1,17 @@ | |||
| // @flow | |||
|
|
|||
| type Format = 'ddmmyy' | 'ddmmyyyy' | 'mmddyy' | 'mmddyyyy' | 'mmyy' | 'mmyyyy'; | |||
|
@kirtisharma1 please resolve Conflict. Also Every select box should have a label. Lets place that at top of the select box. |
| startDate: new Date(), | ||
| endDate: new Date(new Date().setFullYear(new Date().getFullYear() + 50)), | ||
| locale: 'default', | ||
| dateLabel: 'Date', |
There was a problem hiding this comment.
Don't set below as default value, Make them mandatory, Remove the showLabel: false. For accessibility its important that every field has a label.
- dateLabel: 'Date',
- monthLabel: 'Month',
- yearLabel: 'Year',
2 participants
No description provided.