Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(css): adds chartId-based class to dashboard chart holder #19873

Merged
merged 2 commits into from
May 11, 2022

Conversation

rusackas
Copy link
Member

@rusackas rusackas commented Apr 27, 2022

SUMMARY

This PR adds a class to all dashboard charts, referencing their chartID as dashboard-chart-id-{X} as seen here:

Screen_Shot_2022-04-26_at_11_28_15_PM

This allows you to more reliably target a particular chart and do wacky CSS things with it, like so:

Screen Shot 2022-04-26 at 11 26 10 PM

What you do with this new found power is up to you, and at your own risk... but it's easier to do it for a particular chart now, anyway, even if someone moves charts around on a dashboard, which was the main point of fragility in nth-of-type selector approaches.

The world is now your oyster
Screen Shot 2022-04-26 at 11 26 45 PM

TESTING INSTRUCTIONS

Poke around at it! It shouldn't cause any harm to add a new class that has no styles attached to it.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@rusackas
Copy link
Member Author

rusackas commented Apr 27, 2022

@samtfm you may remember this from ages ago... feel free to review, or request changes/additions

@rusackas rusackas changed the title feat: adds chartId-based class to dashboard chart holder feat(css): adds chartId-based class to dashboard chart holder Apr 27, 2022
@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #19873 (c124772) into master (768e4b7) will increase coverage by 12.42%.
The diff coverage is 72.93%.

❗ Current head c124772 differs from pull request most recent head c2421c0. Consider uploading reports for the commit c2421c0 to get more accurate results

@@             Coverage Diff             @@
##           master   #19873       +/-   ##
===========================================
+ Coverage   53.91%   66.33%   +12.42%     
===========================================
  Files        1713     1713               
  Lines       64995    64083      -912     
  Branches     6698     6734       +36     
===========================================
+ Hits        35040    42509     +7469     
+ Misses      28248    19863     -8385     
- Partials     1707     1711        +4     
Flag Coverage Δ
javascript 51.27% <63.28%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...-chart-controls/src/sections/advancedAnalytics.tsx 14.28% <0.00%> (-19.05%) ⬇️
...d/packages/superset-ui-chart-controls/src/types.ts 100.00% <ø> (ø)
...perset-ui-core/src/chart/components/SuperChart.tsx 100.00% <ø> (ø)
...ugins/legacy-plugin-chart-calendar/src/Calendar.js 0.00% <ø> (ø)
...legacy-plugin-chart-calendar/src/ReactCalendar.jsx 0.00% <0.00%> (ø)
...ns/legacy-plugin-chart-heatmap/src/controlPanel.ts 66.66% <ø> (ø)
.../legacy-plugin-chart-histogram/src/controlPanel.ts 60.00% <ø> (ø)
...egacy-plugin-chart-world-map/src/ReactWorldMap.jsx 0.00% <0.00%> (ø)
...gins/legacy-plugin-chart-world-map/src/WorldMap.js 0.00% <ø> (ø)
...acy-preset-chart-deckgl/src/components/Tooltip.tsx 0.00% <0.00%> (ø)
... and 419 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 768e4b7...c2421c0. Read the comment docs.

@michael-s-molina
Copy link
Member

Should we add a comment in the code with the reasoning to prevent anyone in the future to delete this class because it's not being used?

@rusackas
Copy link
Member Author

Should we add a comment in the code with the reasoning to prevent anyone in the future to delete this class because it's not being used?

Excellent suggestion! Thank you for that. I added one, so if it seems sufficiently explanatory, I'd love a (re)review :)

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@suddjian suddjian left a comment

Choose a reason for hiding this comment

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

nice, I like the idea of leaning into the custom css feature by offering official supported hooks and documenting them somewhere.

@rusackas rusackas merged commit 60188ef into master May 11, 2022
@rusackas rusackas deleted the add-class-to-dashboard-with-chart-id branch May 11, 2022 21:44
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
…he#19873)

* feat: adds `chartId`-based class to dashboard chart holder

* Adding comment to denote the purpose of the class in the code.
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/XS 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants