-
Notifications
You must be signed in to change notification settings - Fork 11
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
Timepicker component #31
Changes from 4 commits
5bda6bb
335ec65
3c9bb71
61e0b51
9d09b5e
560a44f
4647d15
47d4062
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,7 +51,12 @@ | |
"module": "lib.esm.js", | ||
"types": "index.d.ts", | ||
"dependencies": { | ||
"@date-io/date-fns": "^1.3.13", | ||
"@date-io/moment": "^1.3.13", | ||
"@material-ui/core": "^4.10.2", | ||
"@material-ui/pickers": "^3.2.10", | ||
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. We would also need this as a peer dependency 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. @Hasmik8 we can add the same in |
||
"date-fns": "^2.16.1", | ||
"moment": "^2.29.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. Again these packages are for date pickers, not time pickers. You'd only need |
||
"react": "^16.9.0", | ||
"react-dom": "^16.9.0" | ||
}, | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,40 @@ | ||||||
import { storiesOf } from '@storybook/react'; | ||||||
import React from 'react'; | ||||||
import { ThemedBackground } from '../../utils/storybook'; | ||||||
import { BaseTimePicker } from './BaseTimePicker'; | ||||||
|
||||||
storiesOf('TimePicker', module) | ||||||
// Litmus Portal | ||||||
.add('Litmus Portal', () => ( | ||||||
<ThemedBackground platform="litmus-portal"> | ||||||
<BaseTimePicker onChange={() => console.log('timepicker')} /> | ||||||
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.
Suggested change
|
||||||
</ThemedBackground> | ||||||
)) | ||||||
|
||||||
// Kubera Chaos | ||||||
.add('Kubera Chaos', () => ( | ||||||
<ThemedBackground platform="kubera-chaos"> | ||||||
<BaseTimePicker onChange={() => console.log('timepicker')} /> | ||||||
</ThemedBackground> | ||||||
)) | ||||||
|
||||||
// Kubera Propel | ||||||
.add('Kubera Propel', () => ( | ||||||
<ThemedBackground platform="kubera-propel"> | ||||||
<BaseTimePicker onChange={() => console.log('timepicker')} /> | ||||||
</ThemedBackground> | ||||||
)) | ||||||
|
||||||
// Kubera Portal | ||||||
.add('Kubera Portal', () => ( | ||||||
<ThemedBackground platform="kubera-portal"> | ||||||
<BaseTimePicker onChange={() => console.log('timepicker')} /> | ||||||
</ThemedBackground> | ||||||
)) | ||||||
|
||||||
// Kubera Core | ||||||
.add('Kubera Core', () => ( | ||||||
<ThemedBackground platform="kubera-core"> | ||||||
<BaseTimePicker onChange={() => console.log('timepicker')} /> | ||||||
</ThemedBackground> | ||||||
)); |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,26 @@ | ||||||
import 'date-fns'; | ||||||
import React, { useState } from 'react'; | ||||||
import MomentUtils from '@date-io/moment'; | ||||||
import { MuiPickersUtilsProvider, TimePicker } from '@material-ui/pickers'; | ||||||
import { BaseTimePickerProps } from './base'; | ||||||
|
||||||
interface TimePickerProps extends BaseTimePickerProps { | ||||||
value?: Date; | ||||||
} | ||||||
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. Time Picker will not take a Date |
||||||
|
||||||
const BaseTimePicker: React.FC<TimePickerProps> = ({ value }) => { | ||||||
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.
Suggested change
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. @S-ayanide no need to pass this value via 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. Date is not required to be passed as props |
||||||
const date = value != undefined ? value : new Date(); | ||||||
const [selectedDate, handleDateChange] = useState<Date | null>(date); | ||||||
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. Date isn't required in time picker 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. @S-ayanide that is , the value shouldn't be passes? 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. No, Date shouldn't be passed |
||||||
return ( | ||||||
<MuiPickersUtilsProvider utils={MomentUtils}> | ||||||
<TimePicker | ||||||
autoOk | ||||||
okLabel="Save" | ||||||
value={selectedDate} | ||||||
data-testid="timepicker" | ||||||
onChange={(newDate: any) => handleDateChange(newDate)} | ||||||
/> | ||||||
</MuiPickersUtilsProvider> | ||||||
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. Can we use a simple KeyboardTimePicker instead! And avoid the MUI utility wrapper if possible!
|
||||||
); | ||||||
}; | ||||||
export { BaseTimePicker }; | ||||||
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.
Suggested change
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,16 @@ | ||||||
import { render } from '@testing-library/react'; | ||||||
import React from 'react'; | ||||||
import { KuberaThemeProvider } from '../../../theme'; | ||||||
import { BaseTimePicker } from '../BaseTimePicker'; | ||||||
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.
Suggested change
|
||||||
|
||||||
describe('Timepicker component', () => { | ||||||
it('Renders', () => { | ||||||
const { getByTestId } = render( | ||||||
<KuberaThemeProvider platform="kubera-chaos"> | ||||||
<BaseTimePicker onChange={() => console.log('timepicker')} /> | ||||||
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.
Suggested change
|
||||||
</KuberaThemeProvider> | ||||||
); | ||||||
|
||||||
expect(getByTestId('timepicker')).toBeTruthy(); | ||||||
}); | ||||||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import { TimePickerProps } from '@material-ui/pickers'; | ||
|
||
export type BaseTimePickerProps = Omit<TimePickerProps, 'value'>; | ||
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. We should be able to use the basic props provided by KeyboardTimePicker, there's no requirement to omit any |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1 @@ | ||||||
export * from './BaseTimePicker'; | ||||||
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.
Suggested change
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { makeStyles } from '@material-ui/core'; | ||
|
||
const useStyles = makeStyles(() => ({})); | ||
|
||
export { useStyles }; | ||
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. If there are no styles required we can simply not have a |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1 +1,2 @@ | ||||||
export * from './Button'; | ||||||
export * from './Timepicker'; | ||||||
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.
Suggested change
Can we also camel case time file name for the same! |
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.
These are packages for Date Picker, we are building a time picker so this is not required here.