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

refactor: icon to icons for syntaxhighlighter and querylist components #15618

Merged
merged 6 commits into from Jul 12, 2021

Conversation

pkdotson
Copy link
Member

@pkdotson pkdotson commented Jul 9, 2021

SUMMARY

This pr migrates the icons in the syntaxhighliger component and querylist.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen Shot 2021-07-09 at 9 18 50 AM

TESTING INSTRUCTIONS

Go to querylist and querypreview modal to test icons

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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 Jul 9, 2021

Codecov Report

Merging #15618 (8a0bc6b) into master (2be52c0) will increase coverage by 0.02%.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15618      +/-   ##
==========================================
+ Coverage   76.88%   76.91%   +0.02%     
==========================================
  Files         976      977       +1     
  Lines       51320    51479     +159     
  Branches     6907     6950      +43     
==========================================
+ Hits        39458    39594     +136     
- Misses      11643    11661      +18     
- Partials      219      224       +5     
Flag Coverage Δ
javascript 71.52% <44.44%> (+0.09%) ⬆️

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

Impacted Files Coverage Δ
...t-frontend/src/views/CRUD/data/query/QueryList.tsx 70.96% <37.50%> (-0.88%) ⬇️
...UD/data/components/SyntaxHighlighterCopy/index.tsx 60.00% <100.00%> (+1.37%) ⬆️
superset-frontend/src/filters/components/common.ts 90.00% <0.00%> (-10.00%) ⬇️
...d/src/filters/components/Time/TimeFilterPlugin.tsx 86.66% <0.00%> (-5.65%) ⬇️
...tend/src/views/CRUD/annotation/AnnotationModal.tsx 60.44% <0.00%> (-2.52%) ⬇️
...s/CRUD/data/database/DatabaseModal/ModalHeader.tsx 89.18% <0.00%> (-2.48%) ⬇️
...rontend/src/views/CRUD/dashboard/DashboardList.tsx 72.46% <0.00%> (-2.17%) ⬇️
...ters/FiltersConfigModal/FiltersConfigForm/state.ts 88.88% <0.00%> (-2.03%) ⬇️
...src/filters/components/Range/RangeFilterPlugin.tsx 89.18% <0.00%> (-1.59%) ⬇️
...c/filters/components/Select/SelectFilterPlugin.tsx 79.82% <0.00%> (-0.88%) ⬇️
... and 35 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 2be52c0...8a0bc6b. Read the comment docs.

@pkdotson
Copy link
Member Author

pkdotson commented Jul 9, 2021

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2021

@pkdotson Ephemeral environment spinning up at http://52.33.109.158:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@rusackas
Copy link
Member

I see the point about the other Emotion approach being possible, but I don't think it's a show-stopper to keep the PR. I'm more excited about killing the Icon component :)

@michael-s-molina
Copy link
Member

michael-s-molina commented Jul 12, 2021

@rusackas I just talked to @pkdotson about this. The idea here is just to simplify the code, removing the double checking of the states.

if (status === 'success') {
   statusConfig.name = (
      <Icons.Check css={theme => StatusIcon(theme, 'running')} />
   );
...

const getColor = (theme: SupersetTheme, status: string) => {
  if (status === 'success') return theme.colors.success.base;
...

@pkdotson is making the changes so please don't merge this yet.

@rusackas
Copy link
Member

Thanks for the update @michael-s-molina - and this is a perfect example of why we love it when people merge their own stuff! There's often something to squeeze in under the wire :)

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jul 12, 2021
@pull-request-size pull-request-size bot added size/M and removed size/L labels Jul 12, 2021
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.

Thanks @pkdotson!

@pkdotson pkdotson merged commit 34542db into apache:master Jul 12, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
apache#15618)

* initial commit

* make changes

* more simplify

* remove code

* add prop

* remove unsued code
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
apache#15618)

* initial commit

* make changes

* more simplify

* remove code

* add prop

* remove unsued code
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
apache#15618)

* initial commit

* make changes

* more simplify

* remove code

* add prop

* remove unsued code
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 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/M 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants