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 #2034

Closed
wants to merge 2 commits into from
Closed

Update DateRange.tsx #2034

wants to merge 2 commits into from

Conversation

Aryam2121
Copy link

Rounding Function: The roundToNearest15Minutes function rounds the selected time to the nearest 15 minutes.
Handle Change Logic: The handleChange function has been modified to use the rounding function before dispatching the updated date values.
Complete Component: The full DateRange component is included for context, with both date and datetime pickers supported.so;ve issue #1452

Rounding Function: The roundToNearest15Minutes function rounds the selected time to the nearest 15 minutes.
Handle Change Logic: The handleChange function has been modified to use the rounding function before dispatching the updated date values.
Complete Component: The full DateRange component is included for context, with both date and datetime pickers supported.so;ve issue Avaiga#1452
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.

Good work @Aryam2121
I think this issue needs a bit more thinking before implementation.
In the code you propose, every component is impacted by the stepping to 15 minutes.
I think this feature should be optional and configurable and should be applied before user selection to limit the available choices.
It should also apply to date_selector and time_selector.
If you provide code, you need also to provide tests.

@@ -6,8 +6,8 @@
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
Copy link
Member

Choose a reason for hiding this comment

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

please do not modify head comment as it is generated and eventually replaced automatically.

@@ -54,7 +62,7 @@ const getRangeDateTime = (
try {
dates = JSON.parse(json);
} catch {
// too bad
// Handle error if JSON parsing fails
Copy link
Member

Choose a reason for hiding this comment

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

we don't want to handle this error

@@ -86,7 +94,7 @@ const DateRange = (props: DateRangeProps) => {
(v: Date | null, start: boolean) => {
setValue((dates) => {
if (v !== null && isValid(v)) {
const newDate = getTimeZonedDate(v, tz, withTime);
const newDate = roundToNearest15Minutes(getTimeZonedDate(v, tz, withTime)); // Round to nearest 15 mins
Copy link
Member

Choose a reason for hiding this comment

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

this behavior should be optional and commanded by a property.
I propose that this property would be set the value in time format (hh:mm:ss) so that we can define before showing the time selector, which times the user can select
I propose the property name: time_step
In this example, time_step=00:15
or time_step=01:00 for letting the user select only time on the hour.
This should also apply to date_selector and time_selector

Comment on lines 229 to 231
width={props.width}
disabled
label={props.labelStart}
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand those changes.

  • Why change the width ?
  • Field has no disabled or label property

Aryam2121 added a commit to Aryam2121/taipy that referenced this pull request 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 Avaiga#2034
@Aryam2121 Aryam2121 mentioned this pull request Oct 14, 2024
@FredLL-Avaiga FredLL-Avaiga added 📈 Improvement Improvement of a feature. 🖰 GUI Related to GUI 🟨 Priority: Medium Not blocking but should be addressed 📝Release Notes Impacts the Release Notes or the Documentation in general labels Oct 17, 2024
Aryam2121 added a commit to Aryam2121/taipy that referenced this pull request Oct 17, 2024
New timeStep Prop: An optional timeStep prop allows the user to define how time is rounded (e.g., to the nearest 15 minutes).
roundToStep Function: This function applies the rounding logic, adjusting the minutes of a selected time to the nearest multiple of the provided timeStep.
handleChange Function: Before dispatching the updated time, it applies the rounding logic if the timeStep prop is provided.solve issue Avaiga#1452 and updated Avaiga#2034
New timeStep Prop: An optional timeStep prop allows the user to define how time is rounded (e.g., to the nearest 15 minutes).
roundToStep Function: This function applies the rounding logic, adjusting the minutes of a selected time to the nearest multiple of the provided timeStep.
handleChange Function: Before dispatching the updated time, it applies the rounding logic if the timeStep prop is provided solve issue Avaiga#1452 and updated code as per review Avaiga#2034
@FredLL-Avaiga
Copy link
Member

Are you still working on this PR @Aryam2121 ?
If you are, please resolve the conflicts, update your branch and answer the questions
If you are not, please close the PR

@Aryam2121
Copy link
Author

i update my Branch already as your requirements you have to check the updated branch i want you review my updated branch

@FredLL-Avaiga
Copy link
Member

i update my Branch already as your requirements you have to check the updated branch i want you review my updated branch

I'll review it once you have fixed the conflicts

@Aryam2121
Copy link
Author

Aryam2121 commented Oct 22, 2024

so you merge this please i already fixed all conflicts

@FredLL-Avaiga
Copy link
Member

so you merge this please i already fixed all conflicts

image

Copy link
Contributor

github-actions bot commented Nov 6, 2024

This PR has been labelled as "🥶Waiting for contributor" because it has been inactive for more than 14 days. If you would like to continue working on this PR, then please add new commit or another comment, otherwise this PR will be closed in 14 days. For more information please refer to the contributing guidelines.

@github-actions github-actions bot added the 🥶Waiting for contributor Issues or PRs waiting for a long time label Nov 6, 2024
@FabienLelaquais
Copy link
Member

Does not do much - and certainly not enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🖰 GUI Related to GUI 📈 Improvement Improvement of a feature. 🟨 Priority: Medium Not blocking but should be addressed 📝Release Notes Impacts the Release Notes or the Documentation in general 🥶Waiting for contributor Issues or PRs waiting for a long time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants