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

chore: refactor AceEditorWrapper to functional component #21532

Merged
merged 8 commits into from
Sep 28, 2022
Merged

chore: refactor AceEditorWrapper to functional component #21532

merged 8 commits into from
Sep 28, 2022

Conversation

EugeneTorap
Copy link
Contributor

SUMMARY

Converted AceEditorWrapper class component into functional component.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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 Sep 20, 2022

Codecov Report

Merging #21532 (dc69bbe) into master (2224ebe) will increase coverage by 0.04%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##           master   #21532      +/-   ##
==========================================
+ Coverage   66.67%   66.71%   +0.04%     
==========================================
  Files        1793     1795       +2     
  Lines       68523    68648     +125     
  Branches     7281     7316      +35     
==========================================
+ Hits        45688    45799     +111     
- Misses      20971    20989      +18     
+ Partials     1864     1860       -4     
Flag Coverage Δ
javascript 52.97% <70.00%> (+0.12%) ⬆️

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

Impacted Files Coverage Δ
...d/src/SqlLab/components/AceEditorWrapper/index.tsx 58.97% <68.08%> (+11.28%) ⬆️
...frontend/src/SqlLab/components/SqlEditor/index.jsx 54.34% <100.00%> (+1.60%) ⬆️
...iews/CRUD/data/dataset/AddDataset/Footer/index.tsx 33.33% <0.00%> (-66.67%) ⬇️
.../plugin-chart-pivot-table/src/plugin/buildQuery.ts 42.85% <0.00%> (-12.70%) ⬇️
...i-chart-controls/src/shared-controls/constants.tsx 22.22% <0.00%> (-11.12%) ⬇️
...nd/plugins/plugin-chart-table/src/controlPanel.tsx 53.12% <0.00%> (-9.84%) ⬇️
.../BigNumber/BigNumberWithTrendline/controlPanel.tsx 16.66% <0.00%> (-8.34%) ⬇️
...ntend/plugins/plugin-chart-table/src/buildQuery.ts 56.25% <0.00%> (-4.77%) ⬇️
...ugin-chart-pivot-table/src/plugin/controlPanel.tsx 4.16% <0.00%> (-2.98%) ⬇️
...et-ui-chart-controls/src/shared-controls/index.tsx 57.14% <0.00%> (-2.48%) ⬇️
... and 44 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@EugeneTorap
Copy link
Contributor Author

@jinghua-qa @andrey-zayats Can you test it?

@andrey-zayats
Copy link

@jinghua-qa @andrey-zayats Can you test it?
What area can this pr affect? (where should we run regression tests?)

@EugeneTorap
Copy link
Contributor Author

SQL Lab editor

@andrey-zayats
Copy link

SQL Lab editor

Okay, I'll take a look

@andrey-zayats
Copy link

/testenv up

@github-actions
Copy link
Contributor

@andrey-zayats Ephemeral environment creation is currently limited to committers.

@jinghua-qa
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@jinghua-qa Ephemeral environment spinning up at http://34.221.133.37:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@andrey-zayats
Copy link

User sees an empty page when trying to open a new tab in SQL lab. The same thing happens when the user follows the link of the saved query.
The issue is already in master

Empty.page.mp4

@EugeneTorap
Copy link
Contributor Author

@andrey-zayats If the issue is already in master then we need to create GitHub issue and fix in a separate PR. Right?

@andrey-zayats
Copy link

@andrey-zayats If the issue is already in master then we need to create GitHub issue and fix in a separate PR. Right?

Yeap, you're right

@andrey-zayats
Copy link

andrey-zayats commented Sep 26, 2022

@EugeneTorap PR LGTM, qa verified.

@zhaoyongjie zhaoyongjie self-requested a review September 27, 2022 12:35
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your patient and hard work.

@rusackas rusackas merged commit 15c3c34 into apache:master Sep 28, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@EugeneTorap EugeneTorap deleted the refactor/AceEditorWrapper-to-fc branch September 28, 2022 02:34
justinpark added a commit to justinpark/superset that referenced this pull request Sep 29, 2022
sadpandajoe added a commit to preset-io/superset that referenced this pull request Oct 17, 2022
sadpandajoe added a commit to preset-io/superset that referenced this pull request Oct 17, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 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 size/L 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants