-
Notifications
You must be signed in to change notification settings - Fork 45
'DateSelector' #161
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
base: develop
Are you sure you want to change the base?
'DateSelector' #161
Changes from 2 commits
3b1c13f
e8fbd78
6893524
963fc8d
169e215
c9f0d5c
67a8b76
43fe181
7110845
38f056d
183ff41
d27736c
0d5f119
3baa202
6e65615
e07bf5a
8b90ec1
c6709dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,193 @@ | ||
// @flow | ||
/** | ||
* | ||
* DateSelector | ||
* | ||
*/ | ||
import React, { useEffect, useState } from 'react'; | ||
import styled from 'styled-components'; | ||
|
||
import classNames from 'classnames'; | ||
import styles from './DateSelector.style'; | ||
import type { Props } from './types'; | ||
import Select from '../../atoms/Select'; | ||
|
||
const monthOptions = [ | ||
'January', | ||
'Febuary', | ||
'March', | ||
'April', | ||
'May', | ||
'June', | ||
'July', | ||
'August', | ||
'September', | ||
'October', | ||
'November', | ||
'December', | ||
]; | ||
|
||
const DateSelector = (props: Props): Node => { | ||
const { className, format, startDate, endDate } = props; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this line at like 31 (props: Props) --> { className, format, startDate, endDate } will achieve same thing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||
const [selectedDate, setSelectedDate] = useState(new Date()); | ||
const [newStartDate, setNewStartDate] = useState( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is startDate and newStartDate diff? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where did you find this 50 or 100 number? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those were for the default start and end date |
||
); | ||
|
||
const setDate = dateType => { | ||
if (dateType === 'start') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed the logic |
||
if (startDate) { | ||
setNewStartDate(new Date(startDate)); | ||
} else { | ||
const d = new Date(); | ||
setNewStartDate(new Date(d.setFullYear(d.getFullYear() - 100))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new Date(d.setFullYear(d.getFullYear() - 100) get repeated too often, create a method for it |
||
} | ||
} else if (dateType === 'end') { | ||
if (endDate) { | ||
setNewEndDate(new Date(endDate)); | ||
} else { | ||
const d = new Date(); | ||
setNewEndDate(new Date(d.setFullYear(d.getFullYear() + 50))); | ||
} | ||
} | ||
}; | ||
|
||
useEffect(() => { | ||
// ... Use hooks | ||
setDate('start'); | ||
setDate('end'); | ||
}, []); | ||
|
||
useEffect(() => { | ||
setSelectedDate(newStartDate); | ||
}, [newStartDate, newEndDate]); | ||
|
||
const getDaysInMonth = () => { | ||
const noOfDays = new Date(selectedDate.getFullYear(), selectedDate.getMonth(), 0).getDate(); | ||
const days = []; | ||
let startDay = 1; | ||
let endDay = noOfDays; | ||
if ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was a mistake. Made changes |
||
selectedDate.getMonth() === newStartDate.getMonth() && | ||
selectedDate.getFullYear() === newStartDate.getFullYear() | ||
) { | ||
startDay = newStartDate.getDate(); | ||
} | ||
if ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still there is redundant code we can reduce, for example:
|
||
selectedDate.getMonth() === newEndDate.getMonth() && | ||
selectedDate.getFullYear() === newEndDate.getFullYear() | ||
) { | ||
endDay = newEndDate.getDate(); | ||
} | ||
for (let i = startDay; i <= endDay; i += 1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these are optional features but to show default start and end dates I'm using these |
||
days.push(i); | ||
} | ||
return days; | ||
}; | ||
|
||
const getMonthOptions = () => { | ||
let months = [...monthOptions]; | ||
if (selectedDate.getFullYear() === newStartDate.getFullYear()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So much of duplication |
||
const startMonth = newStartDate.getMonth(); | ||
months = months.slice(startMonth, months.length); | ||
} | ||
if (selectedDate.getFullYear() === newEndDate.getFullYear()) { | ||
const endMonth = newEndDate.getMonth(); | ||
months = months.slice(0, endMonth + 1); | ||
} | ||
return months; | ||
}; | ||
|
||
const getYearOptions = () => { | ||
const years = []; | ||
for (let i = newStartDate.getFullYear(); i <= newEndDate.getFullYear(); i += 1) { | ||
years.push(i); | ||
} | ||
return years; | ||
}; | ||
|
||
const DaySelector = ( | ||
<Select | ||
onChange={e => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return setSelectedDate( | ||
new Date(selectedDate.getFullYear(), selectedDate.getMonth(), e.target.value) | ||
); | ||
}} | ||
label="" | ||
className={classNames('date', className)} | ||
disabled={false} | ||
elementLocator="date-picker-date" | ||
selectedOption={selectedDate.getDate()} | ||
options={getDaysInMonth()} | ||
/> | ||
); | ||
|
||
const MonthSelector = ( | ||
<Select | ||
onChange={e => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return setSelectedDate( | ||
new Date( | ||
selectedDate.getFullYear(), | ||
monthOptions.indexOf(e.target.value), | ||
selectedDate.getDate() | ||
) | ||
); | ||
}} | ||
label="" | ||
className={classNames('month', className)} | ||
disabled={false} | ||
elementLocator="date-picker-month" | ||
selectedOption={monthOptions[selectedDate.getMonth()]} | ||
options={getMonthOptions()} | ||
/> | ||
); | ||
|
||
const YearSelector = ( | ||
<Select | ||
onChange={e => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return setSelectedDate( | ||
new Date(e.target.value, selectedDate.getMonth(), selectedDate.getDate()) | ||
); | ||
}} | ||
label="" | ||
className={classNames('year', className)} | ||
disabled={false} | ||
elementLocator="date-picker-year" | ||
selectedOption={selectedDate.getFullYear()} | ||
options={getYearOptions()} | ||
/> | ||
); | ||
|
||
return ( | ||
<div className={className}> | ||
{(format.toLowerCase() === 'ddmmyy' || format.toLowerCase() === 'ddmmyyyy') && [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer switch cases There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented |
||
DaySelector, | ||
MonthSelector, | ||
YearSelector, | ||
]} | ||
{(format.toLowerCase() === 'mmddyy' || format.toLowerCase() === 'mmddyyyy') && [ | ||
MonthSelector, | ||
DaySelector, | ||
YearSelector, | ||
]} | ||
{(format.toLowerCase() === 'mmyy' || format.toLowerCase() === 'mmyyyy') && [ | ||
MonthSelector, | ||
YearSelector, | ||
]} | ||
</div> | ||
); | ||
}; | ||
|
||
DateSelector.defaultProps = { | ||
className: 'date-selector', | ||
format: 'ddmmyy', | ||
}; | ||
|
||
export default styled(DateSelector)` | ||
${styles}; | ||
`; | ||
|
||
export { DateSelector as DateSelectorVanilla }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
const dateSelectorDefaultProps = { | ||
className: 'example', | ||
defaultValue: new Date(), | ||
format: 'ddmmyy', | ||
}; | ||
|
||
const mmddyyFormatProps = { | ||
className: 'example', | ||
format: 'mmddyy', | ||
startDate: new Date(), | ||
}; | ||
|
||
const monthYearOnlyProps = { | ||
className: 'example', | ||
format: 'mmyy', | ||
}; | ||
|
||
const startDateProps = { | ||
className: 'example', | ||
format: 'ddmmyy', | ||
startDate: 955865024000, | ||
}; | ||
|
||
const endDateProps = { | ||
className: 'example', | ||
format: 'ddmmyy', | ||
endDate: 2555865024000, | ||
}; | ||
|
||
const startAndEndDateProps = { | ||
className: 'example', | ||
format: 'ddmmyy', | ||
startDate: 955865024000, | ||
endDate: 2555865024000, | ||
}; | ||
|
||
export { | ||
dateSelectorDefaultProps, | ||
mmddyyFormatProps, | ||
monthYearOnlyProps, | ||
startDateProps, | ||
endDateProps, | ||
startAndEndDateProps, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import React from 'react'; | ||
import { storiesOf } from '@storybook/react'; | ||
// import { action } from '@storybook/addon-actions'; | ||
import { | ||
dateSelectorDefaultProps, | ||
mmddyyFormatProps, | ||
monthYearOnlyProps, | ||
startDateProps, | ||
endDateProps, | ||
startAndEndDateProps, | ||
} from './DateSelector.mock'; | ||
|
||
// Import Styled Component to showcase variations | ||
import DateSelector, { DateSelectorVanilla } from '.'; | ||
|
||
storiesOf('Molecules/DateSelector', module) | ||
.addParameters({ jest: ['DateSelector', 'DateSelectorVanilla'] }) | ||
.add('Knobs', () => <DateSelectorVanilla {...dateSelectorDefaultProps} />) | ||
.add('DateSelector format ddmmyyyy', () => <DateSelector {...dateSelectorDefaultProps} />) | ||
.add('DateSelector format mmddyyyy', () => <DateSelector {...mmddyyFormatProps} />) | ||
.add('DateSelector format mmyyyy', () => <DateSelector {...monthYearOnlyProps} />) | ||
.add('DateSelector with Start Date', () => <DateSelector {...startDateProps} />) | ||
.add('DateSelector with End Date', () => <DateSelector {...endDateProps} />) | ||
.add('DateSelector with Start and End Date', () => <DateSelector {...startAndEndDateProps} />); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { css } from 'styled-components'; | ||
|
||
export default css` | ||
${props => (props.inheritedStyles ? props.inheritedStyles : '')}; | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
// @flow | ||
export { default } from './DateSelector'; | ||
export { DateSelectorVanilla } from './DateSelector'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
import React from 'react'; | ||
import { shallow } from 'enzyme'; | ||
|
||
import { DateSelectorVanilla } from '../index'; | ||
import Select from '../../../atoms/Select'; | ||
|
||
describe('<DateSelector />', () => { | ||
let DateSelectorComponent = ''; | ||
beforeEach(() => { | ||
DateSelectorComponent = shallow(<DateSelectorVanilla>Test</DateSelectorVanilla>); | ||
}); | ||
|
||
test('should render correctly', () => { | ||
expect(DateSelectorComponent).toMatchSnapshot(); | ||
}); | ||
|
||
test('should render the 3 Select Components(date month and year)', () => { | ||
DateSelectorComponent.setProps({ | ||
format: 'ddmmyy', | ||
}); | ||
expect(DateSelectorComponent.find(Select)).toHaveLength(3); | ||
}); | ||
|
||
test('should render the 3 Select Components(date month and year)', () => { | ||
DateSelectorComponent.setProps({ | ||
format: 'ddmmyyyy', | ||
}); | ||
expect(DateSelectorComponent.find(Select)).toHaveLength(3); | ||
}); | ||
|
||
test('should render the 3 Select Components(month date and year)', () => { | ||
DateSelectorComponent.setProps({ | ||
format: 'mmddyy', | ||
}); | ||
expect(DateSelectorComponent.find(Select)).toHaveLength(3); | ||
}); | ||
|
||
test('should render the 3 Select Components(month date and year)', () => { | ||
DateSelectorComponent.setProps({ | ||
format: 'mmddyyyy', | ||
}); | ||
expect(DateSelectorComponent.find(Select)).toHaveLength(3); | ||
}); | ||
|
||
test('should render the 2 Select Components(month and year selectors)', () => { | ||
DateSelectorComponent.setProps({ | ||
format: 'mmyy', | ||
}); | ||
expect(DateSelectorComponent.find(Select)).toHaveLength(2); | ||
}); | ||
|
||
test('should render the 2 Select Components(month and year selectors)', () => { | ||
DateSelectorComponent.setProps({ | ||
format: 'mmyyyy', | ||
}); | ||
expect(DateSelectorComponent.find(Select)).toHaveLength(2); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will these change if locale changes to lets say French or Spanish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get this dynamically, from browser itself and then we will no longer need to take care of locales
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implemented