Skip to content

Conversation

@SychevAndrey
Copy link
Contributor

Screenshot 2025-12-25 at 14 52 14 Screenshot 2025-12-25 at 14 51 58

Wroud
Wroud previously approved these changes Dec 26, 2025
let value = displayValue;

if (tableDataContext.useUserFormatting) {
const isDateOnly = /^\d{4}-\d{2}-\d{2}$/.test(displayValue);
Copy link
Member

Choose a reason for hiding this comment

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

const isDateOnly = /^\d{4}-\d{2}-\d{2}$/.test(displayValue);
Is it really required? This will hit performance in the grid noticeably, at least create a regex outside of the render function.
new Date() should be able to parse a date correctly without time. Maybe you can simplify the check by checking the resulting Date object (probably it will have some constant time)

(or just use unified formatter)

Copy link
Member

Choose a reason for hiding this comment

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

Also, it's probably enough to check this format only once for one of the cells because they all will have the same format, or maybe you can check it by column type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can put it on the data context level and set something like "extended datakind" for every column with TIME/DATE/DATETIME values. In the cell we will just get the datakind of the column and use the according formatter. Do you want me refactor like that?

Image

Unfortunately, we can't rely on new Date to check the shape, so testing by regex or splitting is required. However, if we do it only on the TableDataContext level it should be not a problem.

Comment on lines 173 to 175
if (this.extendedDateKinds.has(columnKey.index)) {
return this.extendedDateKinds.get(columnKey.index) as DateTimeKind;
}
Copy link
Member

Choose a reason for hiding this comment

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

probably you want to use ResultSetCacheAction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like an over-engineering to me since we don't need cell or row-level caching here, only the cache by columns

Copy link
Member

Choose a reason for hiding this comment

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

we need to add getColumn like we have getRow in it, it's better because it will clear cache when needed and place it in one place

@SychevAndrey SychevAndrey requested a review from Wroud January 6, 2026 09:25
sergeyteleshev
sergeyteleshev previously approved these changes Jan 6, 2026
Comment on lines 43 to 53
switch (extendedDateKind) {
case DateTimeKind.DateTime:
case DateTimeKind.TimeOnly:
dateFormatter = formattingContext.formatters.dateTime;
break;
case DateTimeKind.DateOnly:
dateFormatter = formattingContext.formatters.dateOnly;
break;
}
const date = new Date(displayValue);
value = dateFormatter!.format(date);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add default formatter here? which is gonna just display something as it is without formatting

cause now this line can break the code:
value = dateFormatter!.format(date);

better to have default switch case to prevent that

Comment on lines 173 to 175
if (this.extendedDateKinds.has(columnKey.index)) {
return this.extendedDateKinds.get(columnKey.index) as DateTimeKind;
}
Copy link
Member

Choose a reason for hiding this comment

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

we need to add getColumn like we have getRow in it, it's better because it will clear cache when needed and place it in one place

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.

4 participants