Timepicker component #31

Hasmik8 · 2020-10-20T12:57:58Z

Description

I've created Timepicker component, which has one prop parameter, which is the value and it's date. It's not a required parameter.

Here are screenshots:

Signed-off-by: Hasmik8 <[email protected]>

src/core/Timepicker/BaseTimePicker/style.ts

src/core/Timepicker/BaseTimePicker/BaseTimePicker.stories.tsx

Signed-off-by: Hasmik8 <[email protected]>

S-ayanide · 2020-10-21T19:22:36Z

package.json

+    "@date-io/date-fns": "^1.3.13",
+    "@date-io/moment": "^1.3.13",


These are packages for Date Picker, we are building a time picker so this is not required here.

S-ayanide · 2020-10-21T19:23:22Z

package.json

+    "date-fns": "^2.16.1",
+    "moment": "^2.29.1",


Again these packages are for date pickers, not time pickers. You'd only need KeyboardTimePicker from @material-ui/pickers

S-ayanide · 2020-10-21T19:26:46Z

package.json

    "@material-ui/core": "^4.10.2",
+    "@material-ui/pickers": "^3.2.10",


We would also need this as a peer dependency

@Hasmik8 we can add the same in "peerDependencies": {}

S-ayanide · 2020-10-21T19:34:18Z

src/core/Timepicker/BaseTimePicker.tsx

+  value?: Date;
+}
+
+const BaseTimePicker: React.FC<TimePickerProps> = ({ value }) => {


Suggested change

const BaseTimePicker: React.FC<TimePickerProps> = ({ value }) => {

const TimePicker: React.FC = () => {

@S-ayanide no need to pass this value via props?

Date is not required to be passed as props

S-ayanide · 2020-10-21T19:34:52Z

src/core/Timepicker/BaseTimePicker.tsx

+interface TimePickerProps extends BaseTimePickerProps {
+  value?: Date;
+}


Time Picker will not take a Date

S-ayanide · 2020-10-21T19:41:21Z

src/core/Timepicker/style.ts

+import { makeStyles } from '@material-ui/core';
+
+const useStyles = makeStyles(() => ({}));
+
+export { useStyles };


If there are no styles required we can simply not have a styles.ts file instead!

S-ayanide · 2020-10-21T19:41:36Z

src/core/Timepicker/index.ts

@@ -0,0 +1 @@
+export * from './BaseTimePicker';


Suggested change

export * from './BaseTimePicker';

export * from './TimePicker';

S-ayanide · 2020-10-21T19:42:17Z

src/core/index.ts

@@ -1 +1,2 @@
 export * from './Button';
+export * from './Timepicker';


Suggested change

export * from './Timepicker';

export * from './TimePicker';

Can we also camel case time file name for the same!

S-ayanide · 2020-10-21T19:43:50Z

src/core/Timepicker/base.ts

+import { TimePickerProps } from '@material-ui/pickers';
+
+export type BaseTimePickerProps = Omit<TimePickerProps, 'value'>;


We should be able to use the basic props provided by KeyboardTimePicker, there's no requirement to omit any

S-ayanide · 2020-10-21T19:44:31Z

src/core/Timepicker/BaseTimePicker.stories.tsx

+  // Litmus Portal
+  .add('Litmus Portal', () => (
+    <ThemedBackground platform="litmus-portal">
+      <BaseTimePicker onChange={() => console.log('timepicker')} />


Suggested change

<BaseTimePicker onChange={() => console.log('timepicker')} />

<BaseTimePicker onChange={() => console.log('Time Changed')} />

S-ayanide · 2020-10-21T19:45:32Z

package-lock.json is not required, can you please delete that! We are using yarn.lock instead

Signed-off-by: Hasmik8 <[email protected]>

S-ayanide · 2020-10-27T11:30:15Z

src/core/Timepicker/BaseTimePicker.tsx

+  value?: Date;
+}
+
+const BaseTimePicker: React.FC<TimePickerProps> = ({ value }) => {


Date is not required to be passed as props

S-ayanide · 2020-10-27T11:30:29Z

src/core/Timepicker/BaseTimePicker.tsx

+  const date = value != undefined ? value : new Date();
+  const [selectedDate, handleDateChange] = useState<Date | null>(date);


No, Date shouldn't be passed

S-ayanide · 2020-10-27T11:30:58Z

Please resolve the above comments as well

Signed-off-by: Hasmik8 <[email protected]>

AVRahul · 2020-10-29T09:43:15Z

As discussed earlier, package-lock.json file is not required as part of this PR. as we are maintaining the yarn.lock instead.

AVRahul · 2020-10-29T09:57:37Z

package.json

    "@material-ui/core": "^4.10.2",
+    "@material-ui/pickers": "^3.2.10",


@Hasmik8 we can add the same in "peerDependencies": {}

Hasmik8 · 2020-10-29T11:12:06Z

@AVRahul I'm running this command npm install --peer @material-ui/pickers and it doesn't add peerDependencies

AVRahul · 2020-10-29T11:15:03Z

@Hasmik8 You can also copy paste the same in peerDependecies manually.

Signed-off-by: Hasmik8 <[email protected]>

Hasmik8 added 2 commits October 20, 2020 15:52

create new timepicker component

5bda6bb

Signed-off-by: Hasmik8 <[email protected]>

fix timepicker base.ts

335ec65

Signed-off-by: Hasmik8 <[email protected]>

Hasmik8 requested review from arkajyotiMukherjee, AVRahul and S-ayanide as code owners October 20, 2020 12:57

AVRahul reviewed Oct 21, 2020

View reviewed changes

src/core/Timepicker/BaseTimePicker/style.ts Outdated Show resolved Hide resolved

src/core/Timepicker/BaseTimePicker/BaseTimePicker.stories.tsx Outdated Show resolved Hide resolved

src/core/Timepicker/BaseTimePicker/BaseTimePicker.stories.tsx Outdated Show resolved Hide resolved

Hasmik8 added adding-component enhancement New feature or request labels Oct 21, 2020

Hasmik8 linked an issue Oct 21, 2020 that may be closed by this pull request

CC: Time Picker #21

Open

S-ayanide assigned Hasmik8 Oct 21, 2020

Hasmik8 added 2 commits October 21, 2020 14:43

fix timepicker issues

3c9bb71

Signed-off-by: Hasmik8 <[email protected]>

fix timepicker issues

61e0b51

Signed-off-by: Hasmik8 <[email protected]>

S-ayanide suggested changes Oct 21, 2020

View reviewed changes

codesniffy bot added the PR: Changes Requested label Oct 21, 2020

fix issues

9d09b5e

Signed-off-by: Hasmik8 <[email protected]>

Hasmik8 removed the PR: Changes Requested label Oct 22, 2020

S-ayanide suggested changes Oct 27, 2020

View reviewed changes

codesniffy bot added the PR: Changes Requested label Oct 27, 2020

update branch

560a44f

Signed-off-by: Hasmik8 <[email protected]>

Hasmik8 removed the PR: Changes Requested label Oct 28, 2020

test error fix

4647d15

Signed-off-by: Hasmik8 <[email protected]>

Hasmik8 added the PR: Review Requested label Oct 29, 2020

AVRahul suggested changes Oct 29, 2020

View reviewed changes

codesniffy bot added the PR: Changes Requested label Oct 29, 2020

update branch && fix package.json

47d4062

Signed-off-by: Hasmik8 <[email protected]>

Hasmik8 removed the PR: Changes Requested label Oct 29, 2020

arkajyotiMukherjee closed this Jan 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timepicker component #31

Timepicker component #31

Hasmik8 commented Oct 20, 2020

S-ayanide Oct 21, 2020

S-ayanide Oct 21, 2020

S-ayanide Oct 21, 2020

AVRahul Oct 29, 2020

S-ayanide Oct 21, 2020

Hasmik8 Oct 22, 2020

S-ayanide Oct 27, 2020

S-ayanide Oct 21, 2020

S-ayanide Oct 21, 2020

S-ayanide Oct 21, 2020

S-ayanide Oct 21, 2020

S-ayanide Oct 21, 2020

S-ayanide Oct 21, 2020

S-ayanide commented Oct 21, 2020

S-ayanide Oct 27, 2020

S-ayanide Oct 27, 2020

S-ayanide commented Oct 27, 2020

AVRahul commented Oct 29, 2020

AVRahul Oct 29, 2020

Hasmik8 commented Oct 29, 2020

AVRahul commented Oct 29, 2020 •

edited

Loading

		"@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",

	const BaseTimePicker: React.FC<TimePickerProps> = ({ value }) => {
	const TimePicker: React.FC = () => {

	export * from './BaseTimePicker';
	export * from './TimePicker';

		@@ -1 +1,2 @@
		export * from './Button';
		export * from './Timepicker';

		import { TimePickerProps } from '@material-ui/pickers';

		export type BaseTimePickerProps = Omit<TimePickerProps, 'value'>;

	<BaseTimePicker onChange={() => console.log('timepicker')} />
	<BaseTimePicker onChange={() => console.log('Time Changed')} />

		const date = value != undefined ? value : new Date();
		const [selectedDate, handleDateChange] = useState<Date \| null>(date);

Timepicker component #31

Timepicker component #31

Conversation

Hasmik8 commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S-ayanide commented Oct 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S-ayanide commented Oct 27, 2020

AVRahul commented Oct 29, 2020

Choose a reason for hiding this comment

Hasmik8 commented Oct 29, 2020

AVRahul commented Oct 29, 2020 • edited Loading

AVRahul commented Oct 29, 2020 •

edited

Loading