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

feat(plugin-chart-table): add column config control #1019

Merged
merged 5 commits into from
Mar 27, 2021

Conversation

ktmud
Copy link
Contributor

@ktmud ktmud commented Mar 22, 2021

🏆 Enhancements

Add column formatting control to table chart, allowing users to change things like column width, text align, number/date formatting per column, all in the Explore view itself (previously you are able to set d3 formats per column, but you'd have to do it via calculated columns or saved metrics).

Also added a separate formatting config for small numbers, so that they can be display as 0 when needed. This is useful when you want to apply human readable SI prefix to big numbers, but don't want to add that to small numbers. When there are both small (abs(x) < 1) and large numbers, users often confuse m for 10-3 with M for 103 (e.g. $1.3m is actually $0.0013, not $1,300,000, the latter would be formatted as $1.3M). By having a separate formatting for small numbers, users can then decide whether to display them as 0 or how many significant digits to use for them.

Must used together with apache/superset#13758 , which passed query responses to chart controls.

After

Screen Shot 2021-03-26 at 12 40 08 AM

column.formatting.demo.mp4

Before

table-chart-custom

@vercel
Copy link

vercel bot commented Mar 22, 2021

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/7YwTThF26FAcKh31pR5iXHr92sxB
✅ Preview: https://superset-ui-git-table-column-formatting-superset.vercel.app

@ktmud ktmud changed the title [WIP] feat(plugin-chart-table): add column config control feat(plugin-chart-table): add column config control Mar 25, 2021
@ktmud ktmud marked this pull request as ready for review March 25, 2021 07:14
@ktmud ktmud requested a review from a team as a code owner March 25, 2021 07:14
@codecov
Copy link

codecov bot commented Mar 25, 2021

Codecov Report

Merging #1019 (2321007) into master (cdd6990) will decrease coverage by 0.24%.
The diff coverage is 20.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1019      +/-   ##
==========================================
- Coverage   27.93%   27.68%   -0.25%     
==========================================
  Files         417      427      +10     
  Lines        8608     8747     +139     
  Branches     1242     1311      +69     
==========================================
+ Hits         2405     2422      +17     
- Misses       6043     6152     +109     
- Partials      160      173      +13     
Impacted Files Coverage Δ
...-chart-controls/src/components/ColumnTypeLabel.tsx 59.09% <0.00%> (-17.38%) ⬇️
...ols/src/components/ControlForm/ControlFormItem.tsx 0.00% <0.00%> (ø)
...hart-controls/src/components/ControlForm/index.tsx 0.00% <0.00%> (ø)
...ui-chart-controls/src/components/ControlHeader.tsx 0.00% <0.00%> (ø)
...erset-ui-chart-controls/src/components/Tooltip.tsx 20.00% <ø> (ø)
packages/superset-ui-chart-controls/src/index.ts 100.00% <ø> (ø)
...omponents/ColumnConfigControl/ColumnConfigItem.tsx 0.00% <0.00%> (ø)
...onents/ColumnConfigControl/ColumnConfigPopover.tsx 0.00% <0.00%> (ø)
.../shared-controls/components/RadioButtonControl.tsx 0.00% <0.00%> (-9.10%) ⬇️
...et-ui-chart-controls/src/shared-controls/index.tsx 38.46% <ø> (-2.60%) ⬇️
... and 29 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 cdd6990...2321007. Read the comment docs.

@kristw
Copy link
Contributor

kristw commented Mar 25, 2021

For the text alignment, can it support center?
Perhaps can use icon if possible.
image

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.

halfway through

label: 'circular',
value: 'circular',
},
['force', t('Force')],
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 related to the table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the signature of the RadioButtonControl to make it more consistent with the SelectControl.

const [isHtml, text, customClassName] = formatValue(column, value);
const style = {
let rounded = value;
if (fractionDigits !== undefined && isNumber && typeof value === 'number') {
Copy link
Contributor

Choose a reason for hiding this comment

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

fractionDigits !== undefined && isNumber can be computed outside of function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can move isNumber out, fractionDigits !== undefined would still be needed here since it can also be zero and I am using its real value later.

if (value === null) {
return [false, 'N/A', 'dt-is-null'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we no longer need this flag?

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, it is moved to inline code as I realized this is not very generalized for formatValue.

https://github.com/apache-superset/superset-ui/pull/1019/files/6f4cca25f8c003631d2e853ee252d32b7dcf8f54#diff-1b5ecf1086dbd4b0e942970dc4e4b20077a2d30d6046d08444b46d8cb57de2a4R271-R275

Also tried to use inline Emotion's css prop but thought manually constructed css classNames are still faster, especially that we need to set it for every cell.

*/
debounceDelay?: number;
} & (T extends 'Select'
? {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it worth extracting the type definition after ? into named 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.

I don't think it's worth it as you'd have to name them and reference them. The indirectness means you'd have to jump back and forth when glancing through code. I also don't see much added value in terms of readability.

const [showAllColumns, setShowAllColumns] = useState(false);

const getColumnInfo = (col: string) => columnConfigs[col] || {};
const setColumnConfig = (col: string, config: T) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

useCallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this matters much in practice. This is only used in an event handler---onChange of ColumnConfigItem, which shouldn't trigger the component's re-render if it's the only prob being updated.

@@ -18,7 +18,7 @@
*/
import React from 'react';
import { Tooltip } from './Tooltip';
import { ColumnTypeLabel } from './ColumnTypeLabel';
import { ColumnTypeLabel, LaxColumnType } from './ColumnTypeLabel';
Copy link
Contributor

Choose a reason for hiding this comment

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

What is Lax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like less strict... Supposedly column types should be mutually exclusive, but we also have some more random types from different places. LaxColumnType makes sure the column type icon component can accept all these types, while more strict ColumnType will be used elsewhere.

@junlincc
Copy link
Contributor

This is great improvement, thank you Jesse! Are you planning to implement more enhancements to table chart?
@mihir174 let's revisit the control layout and popover when both tables are done.

cc @kgabryje @zhaoyongjie

@ktmud
Copy link
Contributor Author

ktmud commented Mar 26, 2021

For the text alignment, can it support center?
Perhaps can use icon if possible.
image

Updated to fontawesome icons.

@ktmud
Copy link
Contributor Author

ktmud commented Mar 26, 2021

@junlincc I don't have any other major work planned for table viz.

@kgabryje
Copy link
Contributor

@junlincc I don't have any other major work planned for table viz.

After this PR is merged, I'll work on implementing totals if you don't mind

@junlincc
Copy link
Contributor

@kristw Hey Krist, is this PR ready to move forward? we are happy to take it from here. 🙏

@kristw
Copy link
Contributor

kristw commented Mar 27, 2021

@kristw Hey Krist, is this PR ready to move forward? we are happy to take it from here. 🙏

Thanks. I think I'm good with the current.

@ktmud ktmud merged commit 2af6a24 into master Mar 27, 2021
@delete-merged-branch delete-merged-branch bot deleted the table-column-formatting branch March 27, 2021 00:52
@ktmud
Copy link
Contributor Author

ktmud commented Mar 27, 2021

Merging and will do a follow up PR if there are any bugs surfacing up while testing.

@junlincc
Copy link
Contributor

junlincc commented Apr 29, 2021

Is the expected behavior - stretch to fit fit screen by default? @ktmud
Screen Shot 2021-04-28 at 10 44 33 PM

Screen.Recording.2021-04-28.at.10.48.37.PM.mov

@junlincc
Copy link
Contributor

Currency format only support $, we may wanna expand support for top 5 currency. add it to backlog
Screen Shot 2021-04-28 at 11 32 09 PM

@junlincc
Copy link
Contributor

junlincc commented Apr 29, 2021

A couple more issues..

  1. SHOW CELL BARS is check by default in the popover, but it's not applied to the column by default, user needs to uncheck then check to see changes
  2. Without re-checking the SHOW CELL BARS box first, ALIGN +/- AND COLOR +/- doesn't really work.
Screen.Recording.2021-04-28.at.11.47.20.PM.mov

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.

4 participants