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

X2-9506 added future date range for relative date range picker #340

Merged
merged 14 commits into from
Oct 3, 2024

Conversation

SemenStruchev
Copy link
Contributor

No description provided.

@@ -128,6 +101,14 @@ const handlers = {
};
},

[options.NEXT_DAY]: (timezone) => {
const nextDay = now(null, timezone).add(1, "day"); // Added
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const nextDay = now(null, timezone).add(1, "day"); // Added
const nextDay = now(null, timezone).add(1, "day");

@@ -150,6 +131,14 @@ const handlers = {
};
},

[options.NEXT_WEEK]: (timezone) => {
const nextWeek = now(null, timezone).add(7, "day");
Copy link
Member

Choose a reason for hiding this comment

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

This maybe better for readability and consistency since you're using 'week' below.

Suggested change
const nextWeek = now(null, timezone).add(7, "day");
const nextWeek = now(null, timezone).add(1, "week");

Check if this is different from (7, "days")

@@ -191,6 +188,14 @@ const handlers = {
};
},

[options.NEXT_QUARTER]: (timezone) => {
const nextQuarter = now(null, timezone).add(3, "month");
Copy link
Member

Choose a reason for hiding this comment

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

Can you do .add(1, "quarter") here?

Copy link
Contributor

@KarthickXola KarthickXola left a comment

Choose a reason for hiding this comment

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

there is lint issue here, and seller app also due to this

@@ -220,13 +233,22 @@ export const RelativeDateRange = ({
onChange,
onSubmit,
timezoneName,
isFutureDatesAllowed = false, // New prop
Copy link
Contributor

Choose a reason for hiding this comment

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

// New prpo => maynot needed.
you can add comment of what it does, or add in Ui lit story board https://ui.xola.io/?path=/docs/data-display-date-time-date-range-picker--relative-date-ranges

const filterFutureDates = (options) => {
if (!isFutureDatesAllowed) {
// Filter out future date options
return options.filter((option) => !option.value.includes("next"));
Copy link
Contributor

Choose a reason for hiding this comment

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

can you check with Future date instead of checking just "next" text
const futureDates = [options.NEXT_QUARTER, ....] and check if not exist here

Copy link
Contributor

@KarthickXola KarthickXola left a comment

Choose a reason for hiding this comment

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

Lint issue too

@@ -258,4 +284,5 @@ RelativeDateRange.propTypes = {
showApply: PropTypes.bool,
value: PropTypes.string,
timezoneName: PropTypes.string,
isFutureDatesAllowed: PropTypes.bool, // New prop type
Copy link
Contributor

Choose a reason for hiding this comment

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

same // New prpo => maynot needed.
you can add comment of what it does, or add in Ui lit story board https://ui.xola.io/?path=/docs/data-display-date-time-date-range-picker--relative-date-ranges

Copy link
Contributor

@KarthickXola KarthickXola left a comment

Choose a reason for hiding this comment

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

lgtm
@rushi can you check once

}) => {
const handleChange = (e) => {
const rangeName = e.target.value;
const range = handlers[rangeName](timezoneName);
onChange(rangeName, range);
};

const filterFutureDates = (options) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this func. we are try moving out from component

Copy link
Member

Choose a reason for hiding this comment

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

yes please. move it out of the component.

@ranjantanya ranjantanya merged commit 6d617ac into xola:master Oct 3, 2024
2 checks passed
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.

5 participants