Skip to content
New issue

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

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

Already on GitHub? # to your account

chore: removing use of supersetTheme in favor of ThemeProvider #17000

Merged
merged 1 commit into from
Oct 7, 2021

Conversation

rusackas
Copy link
Member

@rusackas rusackas commented Oct 7, 2021

SUMMARY

For ErrorAlert and BasicErrorAlert this PR removes direct use of supersetTheme in favor of the useTheme hook and the ThemeProvider component.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

there should be no discernable change

TESTING INSTRUCTIONS

Make sure errors look OK in the ephemeral environment.

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

@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #17000 (f85f8e7) into master (cde4cdc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #17000   +/-   ##
=======================================
  Coverage   76.93%   76.93%           
=======================================
  Files        1030     1030           
  Lines       55031    55033    +2     
  Branches     7465     7465           
=======================================
+ Hits        42336    42340    +4     
+ Misses      12441    12439    -2     
  Partials      254      254           
Flag Coverage Δ
hive 81.45% <ø> (ø)
javascript 70.90% <100.00%> (+<0.01%) ⬆️
mysql 81.90% <ø> (ø)
postgres 81.91% <ø> (ø)
presto 81.78% <ø> (+<0.01%) ⬆️
python 82.42% <ø> (+<0.01%) ⬆️
sqlite 81.58% <ø> (ø)

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

Impacted Files Coverage Δ
...nd/src/components/ErrorMessage/BasicErrorAlert.tsx 100.00% <100.00%> (ø)
...rontend/src/components/ErrorMessage/ErrorAlert.tsx 94.23% <100.00%> (+0.11%) ⬆️
superset/db_engine_specs/presto.py 90.37% <0.00%> (+0.41%) ⬆️

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 cde4cdc...f85f8e7. Read the comment docs.

Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

Lgtm

@villebro
Copy link
Member

villebro commented Oct 7, 2021

What on earth is pullapprove? I can't find any references to it in our CI confs..
image

@villebro villebro merged commit 66fbce9 into master Oct 7, 2021
@villebro villebro deleted the no-supersetTheme-in-errors branch October 7, 2021 07:37
@kgabryje
Copy link
Member

kgabryje commented Oct 7, 2021

What on earth is pullapprove? I can't find any references to it in our CI confs.. image

I haven't seen it before either...

@villebro
Copy link
Member

villebro commented Oct 7, 2021

What on earth is pullapprove? I can't find any references to it in our CI confs.. ![image](https://user->
I haven't seen it before either...

I'm going to do some quick investigation into this

@villebro
Copy link
Member

villebro commented Oct 7, 2021

It appears this has been added some time ago, but never implemented: https://issues.apache.org/jira/browse/INFRA-18241 I wonder if we should just pull it (pun intended)?

@villebro
Copy link
Member

villebro commented Oct 7, 2021

Actually, let's not remove it, this could be leveraged to customize our PR review process more. I'll open a draft to test how this could be used.

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/S 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants