Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

fix(plugin-chart-table): sort and search time column #669

Merged
merged 5 commits into from Jul 8, 2020

Conversation

ktmud
Copy link
Contributor

@ktmud ktmud commented Jul 6, 2020

🐛 Bug Fix

  1. Fix getTime undefined error when sorting a time column in table chart. Also refactors how datetime is handled for react-table search, sort and render methods.

  2. Fix spacing issue for table pagination in dashboards

    Before:

    After:

Test Plan

Unit test in superset-ui and E2E test in superset-frontend.

cc @serenajiang @kristw

@ktmud ktmud requested a review from a team as a code owner July 6, 2020 22:37
@vercel
Copy link

vercel bot commented Jul 6, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/lfpneol1e
✅ Preview: https://superset-ui-git-fork-ktmud-table-sort.superset.vercel.app

@codecov
Copy link

codecov bot commented Jul 6, 2020

Codecov Report

Merging #669 into master will increase coverage by 0.80%.
The diff coverage is 95.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #669      +/-   ##
==========================================
+ Coverage   23.91%   24.71%   +0.80%     
==========================================
  Files         336      340       +4     
  Lines        7661     7748      +87     
  Branches      948      981      +33     
==========================================
+ Hits         1832     1915      +83     
- Misses       5742     5743       +1     
- Partials       87       90       +3     
Impacted Files Coverage Δ
...t-ui-query/src/api/legacy/getDatasourceMetadata.ts 100.00% <ø> (ø)
...ckages/superset-ui-query/src/extractQueryFields.ts 100.00% <ø> (ø)
...kages/superset-ui-query/src/types/QueryFormData.ts 100.00% <ø> (ø)
...s/superset-ui-query/test/api/setupClientForTest.ts 100.00% <ø> (ø)
.../plugin-chart-table/src/utils/DateWithFormatter.ts 63.63% <63.63%> (ø)
plugins/plugin-chart-table/src/transformProps.ts 71.60% <91.66%> (-0.35%) ⬇️
.../superset-ui-connection/src/SupersetClientClass.ts 100.00% <100.00%> (ø)
...ages/superset-ui-connection/src/callApi/callApi.ts 100.00% <100.00%> (ø)
...uperset-ui-connection/src/callApi/parseResponse.ts 100.00% <100.00%> (ø)
packages/superset-ui-connection/src/constants.ts 100.00% <100.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 653f3d9...f9eeb7c. Read the comment docs.

Copy link
Contributor

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

a couple questions


toString(): string {
if (this.formatter === String) {
return String(this.input);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this should be return this.formatter(this.input);?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's basically the same thing?


// force UTC time for timestamps without a timezone
if (forceUTC && typeof value === 'string' && REGEXP_TIMESTAMP_NO_TIMEZONE.test(value)) {
value = `${value}Z`;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this correct? I thought you needed to replace the T with a Z to convert the string to UTC time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

T is the separator between date and time. I don't think I've ever seen timestamps like 2020-07-07Z18:20:02.495.

Copy link
Contributor

@kristw kristw left a comment

Choose a reason for hiding this comment

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

I am not sure about extending Date but seems like ok for the requirement.

plugins/plugin-chart-table/src/transformProps.ts Outdated Show resolved Hide resolved
@ktmud
Copy link
Contributor Author

ktmud commented Jul 8, 2020

I am not sure about extending Date but seems like ok for the requirement.

Yeah, I wanted (or had) to use Date.getTime() value for sorting, but the formatted string for searching. This is the cleanest solution I can think of.

@pull-request-size pull-request-size bot removed the size/M label Jul 8, 2020
@vercel vercel bot temporarily deployed to Preview July 8, 2020 00:41 Inactive
@vercel vercel bot temporarily deployed to Preview July 8, 2020 00:45 Inactive
@ktmud ktmud merged commit 95a2dd2 into apache-superset:master Jul 8, 2020
@ktmud ktmud deleted the table-sort branch July 8, 2020 02:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants