-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix(ui): UI fixes #1945
fix(ui): UI fixes #1945
Conversation
@@ -148,15 +149,27 @@ function ResultDetails() { | |||
}, | |||
); | |||
|
|||
// !NOTE: Workaround to display the date in the correct format, to be replaced by a proper solution. | |||
const dateTimeFromIndex = useMemo(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add it in usePromise
instead of creating a useMemo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to not mix it with the matrix promise and keep clear separation of concerns. For the useMemo
, the index
array in matrixRes.data
is large, making date formatting expensive. If we trigger a filter for example it will be re-calculated.
don't you think we should keep it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's workaround before the backend fix it, I think it's correct to place it in the usePromise.
As you want.
@@ -6,7 +6,7 @@ export const Root = styled(Box)(({ theme }) => ({ | |||
display: "flex", | |||
flexDirection: "column", | |||
alignItems: "center", | |||
overflow: "auto", | |||
overflow: "hidden", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you don't break anything? Test with big matrix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handsontable docs recommands the parent container overflow is set to "hidden", normally it doesn't break anything i've tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc recommands auto
or hidden
. If you're sure ok.
No description provided.