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

feat: allow toggling of table viz's bar chart backgrounds #352

Merged
merged 3 commits into from
Apr 13, 2020

Conversation

rusackas
Copy link
Member

@rusackas rusackas commented Apr 10, 2020

🏆 Enhancements

Fixes, in coordination with this parallel PR, Superset issue #9499

@vercel
Copy link

vercel bot commented Apr 10, 2020

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

🔍 Inspect: https://zeit.co/superset/superset-ui/ear00opxw
✅ Preview: https://superset-ui-git-fork-preset-io-issue9499.superset.now.sh

@codecov
Copy link

codecov bot commented Apr 10, 2020

Codecov Report

Merging #352 into master will decrease coverage by 39.91%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #352       +/-   ##
===========================================
- Coverage   91.67%   51.76%   -39.92%     
===========================================
  Files         118      122        +4     
  Lines        1489     2639     +1150     
  Branches      388      391        +3     
===========================================
+ Hits         1365     1366        +1     
- Misses         95     1244     +1149     
  Partials       29       29               
Impacted Files Coverage Δ
plugins/table/src/transformProps.ts 52.00% <0.00%> (-2.17%) ⬇️
plugins/table/test/testData.ts 100.00% <ø> (ø)
plugins/table/src/ReactDataTable.tsx 79.76% <66.66%> (-0.73%) ⬇️
...legacy-plugin-chart-calendar/src/transformProps.js 0.00% <0.00%> (ø)
...ugins/legacy-plugin-chart-calendar/src/Calendar.js 0.00% <0.00%> (ø)
plugins/legacy-plugin-chart-calendar/src/index.js 0.00% <0.00%> (ø)
...cy-plugin-chart-calendar/src/vendor/cal-heatmap.js 0.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 308f2ab...d725783. Read the comment docs.

@rusackas rusackas requested a review from kristw April 10, 2020 22:33
@rusackas rusackas marked this pull request as ready for review April 10, 2020 22:33
@rusackas rusackas requested a review from a team as a code owner April 10, 2020 22:33
@rusackas
Copy link
Member Author

@krist this one is minor, but would love to know how you prefer to approach these double-PRs as a coordinated effort. We can discuss on Slack if it's easier :D

return (
<td
key={key}
// only set innerHTML for actual html content, this saves time
dangerouslySetInnerHTML={isHtml ? { __html: text } : undefined}
data-sort={val}
className={keyIsMetric ? 'text-right' : ''}
className={showCellBar ? 'text-right' : ''}
Copy link
Contributor

Choose a reason for hiding this comment

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

It probably makes sense to always align metrics to the right even if we don't need to show the bar chart?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, that's fine... it looks more wrong than right to me to have them right-aligned in many cases, but that all depends on the number formatting. I guess there's no need to squeeze that extra change, as it wasn't part of the original issue. I'll change it back.

@rusackas rusackas marked this pull request as draft April 12, 2020 05:22
@rusackas
Copy link
Member Author

Thanks for the review, @ktmud :)

If you don't mind reviewing/approving the superset PR, I'll merge both, unless you see any deployment problems in doing so.

@ktmud ktmud merged commit 5d5ade9 into apache-superset:master Apr 13, 2020
@rusackas rusackas deleted the issue9499 branch April 13, 2020 17:15
@kristw
Copy link
Contributor

kristw commented Apr 13, 2020

published as 0.12.14

kristw pushed a commit that referenced this pull request Apr 17, 2020
@rusackas rusackas restored the issue9499 branch April 22, 2020 17:05
@rusackas rusackas deleted the issue9499 branch April 27, 2020 23:16
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.

3 participants