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.

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

Update DateRange.tsx #2050

Closed
wants to merge 1 commit into from
Closed

Update DateRange.tsx #2050

wants to merge 1 commit into from

Conversation

Aryam2121
Copy link

@Aryam2121 Aryam2121 commented Oct 14, 2024

timeStep Prop: Control the rounding by passing a timeStep prop (e.g., 15 for 15-minute intervals).
Testing: Implement unit tests outside of this file to validate the functionality and edge cases of rounding behavior.
UI Considerations: The layout remains responsive and adjusts to the presence or absence of withTime.solve issue #1452 and old pull request changes review #2034

timeStep Prop: Control the rounding by passing a timeStep prop (e.g., 15 for 15-minute intervals).
Testing: Implement unit tests outside of this file to validate the functionality and edge cases of rounding behavior.
UI Considerations: The layout remains responsive and adjusts to the presence or absence of withTime.solve issue Avaiga#2034
Copy link
Member

@FredLL-Avaiga FredLL-Avaiga left a comment

Choose a reason for hiding this comment

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

this is not finished I guess ?

}
return [null, null];
// Function to round the time based on the given step
const roundToStep = (date: Date | null, step: number | undefined): Date | null => {
Copy link
Member

Choose a reason for hiding this comment

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

Should go in a dateUtils.ts so that we can reuse it in dateSelector.tsx and timeSelector.tsx ...

const roundToStep = (date: Date | null, step: number | undefined): Date | null => {
if (!date || !step) return date;
const roundedMinutes = Math.ceil(date.getMinutes() / step) * step;
return setMinutes(setHours(date, date.getHours()), roundedMinutes);
Copy link
Member

Choose a reason for hiding this comment

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

what happens if step = 40 and current min = 50 ?
roundedMinutes would be 80
How does DateFns handles this ?

@@ -231,7 +218,6 @@ const DateRange = (props: DateRangeProps) => {
/>
</>
)}
{props.children}
Copy link
Member

Choose a reason for hiding this comment

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

Why ?

}

const textFieldProps = { textField: { margin: "dense" } } as BaseDateTimePickerSlotProps<Date>;

const getRangeDateTime = (
Copy link
Member

Choose a reason for hiding this comment

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

Why ?

const dispatch = useDispatch();
const formatConfig = useFormatConfig();
const tz = formatConfig.timeZone;
const [value, setValue] = useState<[Date | null, Date | null]>([null, null]);
const [startProps, setStartProps] = useState<DateProps>({});
const [endProps, setEndProps] = useState<DateProps>({});
const module = useModule();
Copy link
Member

Choose a reason for hiding this comment

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

Why ?

@FredLL-Avaiga
Copy link
Member

Are you still working on this @Aryam2121 ?
if you do, update your branch, resolve the conflicts and answer the questions, tx.
if not please close the PR.

@FredLL-Avaiga
Copy link
Member

Are you still working on this @Aryam2121 ? if you do, update your branch, resolve the conflicts and answer the questions, tx. if not please close the PR.

WIthout a reply from you @Aryam2121, we'll have to close this PR ...

@Aryam2121
Copy link
Author

I am updated already

@FredLL-Avaiga
Copy link
Member

can you resolve the conflicts and fix the failing checks ?

@FredLL-Avaiga
Copy link
Member

no activity, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants