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: Stop editor scrolling to top #26754

Merged
merged 9 commits into from
Jan 25, 2024

Conversation

puridach-w
Copy link
Contributor

SUMMARY

This contribution introduces a vital usability upgrade to the SQL Editor within Apache Superset. The enhancement consists of a coding modification that embeds the cursor position into the Redux state.

Traditionally, every time users switch between SQL Lab tabs, the SQL editor viewport resets, forcing them to scroll from the top to locate their last worked-on position. This scenario presents an inconvenience that disrupts workflow and decreases editing efficiency.

This update directly addresses the issue. Upon rendering the SQL editor, it now auto-focuses on the saved cursor position from the Redux state, aligning the screen with the user's last point of work. Users can now effortlessly switch tabs, and upon their return, find the SQL Editor positioned exactly where they left off.

BEFORE SCREENSHOTS OR ANIMATED GIF

Before.-.stop.editor.mov

AFTER SCREENSHOTS OR ANIMATED GIF

After.-.stop.editor.mov

TESTING INSTRUCTIONS

  • Navigate to the SQLLab page
  • Write some query statement
  • Switch to other SQL editor tab
  • Switch back to the original SQL editor tab
  • Verify that cursor focus on exact location where it was placed before you switched tabs

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

const { row, column } = cursorPosition;
editor.moveCursorToPosition({ row, column });
editor.focus();
editor.renderer.updateFontSize();
Copy link
Member

Choose a reason for hiding this comment

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

This operation(updateFontSize) seems redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I already modify the code to remove that line of code.

Copy link

codecov bot commented Jan 24, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (04445a3) 69.48% compared to head (48d7a53) 69.49%.
Report is 31 commits behind head on master.

Files Patch % Lines
superset-frontend/src/SqlLab/reducers/sqlLab.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #26754   +/-   ##
=======================================
  Coverage   69.48%   69.49%           
=======================================
  Files        1894     1894           
  Lines       74151    74165   +14     
  Branches     8243     8244    +1     
=======================================
+ Hits        51527    51539   +12     
- Misses      20555    20557    +2     
  Partials     2069     2069           
Flag Coverage Δ
javascript 56.87% <92.30%> (+0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

puridach-w and others added 5 commits January 25, 2024 10:44
Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>
….tsx

Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>
Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>
…m/puridach-w/superset into feat/stop-editor-scrolling-to-top

# Conflicts:
#	superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx
Copy link
Member

@justinpark justinpark 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 the contribution.

@rusackas rusackas merged commit ed934a9 into apache:master Jan 25, 2024
26 checks passed
eschutho pushed a commit to preset-io/superset that referenced this pull request Jan 31, 2024
Co-authored-by: Puridach Wutthihathaithamrong <>
Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
Co-authored-by: Puridach Wutthihathaithamrong <>
Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.0.0 labels Apr 17, 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/M 🚢 4.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants