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

fix(legacy-plugin-chart-pivot-table): fix displaying image inside rows #954

Merged
merged 6 commits into from
Feb 16, 2021

Conversation

mayurnewase
Copy link
Contributor

@mayurnewase mayurnewase commented Feb 11, 2021

🐛 Bug Fix
fixes apache/superset#12681

This ensures text content of cell is only updated if its not empty,as it also overrides innerHtml property of element.

@mayurnewase mayurnewase requested a review from a team as a code owner February 11, 2021 05:04
@vercel
Copy link

vercel bot commented Feb 11, 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/bfh4giue3
✅ Preview: https://superset-ui-git-fork-mayurnewase-fix-superset-12681.superset.now.sh

@mayurnewase mayurnewase changed the title fix fix(legacy-plugin-chart-pivot-table): fix displaying image inside rows Feb 11, 2021
@codecov
Copy link

codecov bot commented Feb 11, 2021

Codecov Report

Merging #954 (a24710a) into master (18f45b6) will decrease coverage by 0.00%.
The diff coverage is 8.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #954      +/-   ##
==========================================
- Coverage   27.67%   27.67%   -0.01%     
==========================================
  Files         401      401              
  Lines        8249     8250       +1     
  Branches     1138     1139       +1     
==========================================
  Hits         2283     2283              
- Misses       5830     5831       +1     
  Partials      136      136              
Impacted Files Coverage Δ
.../legacy-plugin-chart-pivot-table/src/PivotTable.js 0.00% <0.00%> (ø)
...-plugin-chart-pivot-table/src/utils/formatCells.ts 69.56% <100.00%> (-1.27%) ⬇️
...ckages/superset-ui-chart-controls/src/sections.tsx 100.00% <0.00%> (ø)
...ges/superset-ui-core/src/query/buildQueryObject.ts 100.00% <0.00%> (ø)
.../superset-ui-core/src/query/types/QueryFormData.ts 100.00% <0.00%> (ø)

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 18f45b6...a24710a. Read the comment docs.

Copy link
Contributor

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Could we rather check for $(this)[0].children.length here? If it's 0 we can assume there are no html tags.

@ktmud
Copy link
Contributor

ktmud commented Feb 11, 2021

An easier solution to support the hack in apache/superset#12681 (including hyperlinks) is to check whether cell children is a single text node (this.childNodes.length == 1 && this.childNodes[0].nodeType === Node.TEXT_NODE), and only run formatCellValue if it is.

The assumption is if someone is advanced enough to add HTML to a cell, they are probably OK with not applying d3 formatting to it.

@ktmud
Copy link
Contributor

ktmud commented Feb 11, 2021

Check out this branch: https://github.com/apache-superset/superset-ui/compare/pivot-table-fix

It seems sorting on PivotTable is also broken by #880 so I fixed it, too.

@junlincc
Copy link
Contributor

@villebro could you take another look, we need to get the fix out soon. @maloun96 feel free to review any PRs in this repo too. 🙏

@mayurnewase thanks for fixing it!

Copy link
Contributor

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

LGTM.

@mayurnewase I also added a couple of HTML tests cases with in Storybook. Hope you don't mind.

@villebro villebro merged commit 03c91b4 into apache-superset:master Feb 16, 2021
ktmud added a commit that referenced this pull request Feb 18, 2021
#954)

* fix

* lint

* lint

* complete fix

* remove log

* Add storybook entry for html

Co-authored-by: Jesse Yang <jesse.yang@airbnb.com>
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.

[chart]Pivot Table Rendering Superset 1.0
4 participants