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

UCL-563 Data grid datetime picker #748

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

UCL-563 Data grid datetime picker #748

wants to merge 21 commits into from

Conversation

szmejk
Copy link
Contributor

@szmejk szmejk commented Dec 4, 2024

No description provided.

@szmejk szmejk marked this pull request as ready for review December 11, 2024 14:53
@szmejk szmejk requested a review from a team as a code owner December 11, 2024 14:53
Copy link

sonarcloud bot commented Dec 11, 2024

package.json Outdated
@@ -134,5 +134,8 @@
"ts-jest": "^29.1.5",
"tslib": "^2.6.3",
"typescript": "^5.5.3"
},
"dependencies": {
"date-fns": "^4.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it should be a peer dependency too? @pawel-napieracz wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const { dataGrid, getAllByRole } = createDataGridWithDateFilter();

expect(dataGrid).toBeInTheDocument();

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

await userEvent.clear(toInput);
await userEvent.type(toInput, "not a date[Tab]");

expect(getAllByText("The format must be yyyy-mm-dd hh:mm:ss")).toHaveLength(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would split the test into custom dates / field validation, and add more error cases (like the ones reported by Scott

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

<Fragment>
<Icon icon={Icons.Calendar} />
<Typography variant="body" className={classes["caption"]}>
Date is <b>{dateTagCaption}</b>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to localize all the texts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

to: "To",
cancel: "Cancel",
apply: "Apply",
dateInputPlaceholder: "yyyy-mm-dd hh:mm:ss"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, extract to a static and refer to the format in other places (such as above, in the error message)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return (
<Fragment>
<InputWrapper
style={{ marginTop: 0, maxWidth: "224px" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why passing the style like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

<InputWrapper
style={{ marginTop: 0, maxWidth: "224px" }}
label={from}
name={"fromDate"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name={"fromDate"}
name="fromDate"

to me it's ok, but @pawel-napieracz hunts me with this :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -66,6 +76,7 @@ export const DatePicker = ({ onSelect, value, required, mode, locale, ...rest }:
{...rest}
{...commonProps}
mode={mode}
endMonth={new Date(3000, 12)}
Copy link
Contributor

Choose a reason for hiding this comment

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

A prophecy...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

discussed on slack

],
isLoading: false,
enableMultiSorting: true
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add the entry with play function that will open the filter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
}
};
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

Some months take more height than others. This makes the entire modal "jump" a little. I don't think it's a big issue but would be nice to discuss with the UX.

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

Successfully merging this pull request may close these issues.

2 participants