feat: Draggable and Resizable Modal #16394

geido · 2021-08-22T21:20:14Z

SUMMARY

This PR introduces four new options to the Modal component

resizable: It makes the Modal resizable using re-resizable
resizableConfig: It accepts a re-resizable configuration object
draggable: It makes the Modal draggable using react-draggable
resizableConfig: It accepts a react-draggable configuration object

The resizable and draggable are completely independent of each other, they can be used in combination or not used at all.

Note: When resizable or/and draggable are used, the Modal mask will be disabled.

Note: When resizable and draggable are not used, the original Modal node will be returned. This is to minimize the impact of these changes on existing Modals.

SHOULD I USE THESE OPTIONS?

This is up for debate. Personally, I believe resizable and draggable Modals can be useful in very specific cases but should never be used heavily. A better UX solution should be found when the need of resizable and draggable Modals increases.

FIRST EXAMPLE: VIEW QUERY MODAL

Both resizable and draggable options have been enabled for the "View Query" Modal. The Modal has been slightly enhanced in order for the SyntaxHighlighter Component to take the full height.

BEFORE

AFTER

draggable_resizable.mp4

TESTING INSTRUCTIONS

Open Explore
Click on "View Query"
Hover the title and drag the Modal in the window
Hover and drag the right, right bottom, and bottom corners to resize the modal

ADDITIONAL INFORMATION

Has associated issue: Fixes [Viewquery]Allow View Query Screen to move around #16205
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

geido · 2021-08-22T21:28:51Z

@rosemarie-chiu @jinghua-qa

codecov · 2021-08-22T21:45:59Z

Codecov Report

Merging #16394 (7866549) into master (c14364c) will decrease coverage by 0.02%.
The diff coverage is 73.07%.

@@            Coverage Diff             @@
##           master   #16394      +/-   ##
==========================================
- Coverage   76.55%   76.52%   -0.03%     
==========================================
  Files        1000     1000              
  Lines       53500    53537      +37     
  Branches     6819     6843      +24     
==========================================
+ Hits        40956    40969      +13     
- Misses      12306    12330      +24     
  Partials      238      238

Flag	Coverage Δ
javascript	`70.65% <73.07%> (-0.04%)`	⬇️

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

Impacted Files	Coverage Δ
...set-frontend/src/components/ModalTrigger/index.jsx	`95.65% <ø> (ø)`
.../components/ExploreAdditionalActionsMenu/index.jsx	`100.00% <ø> (ø)`
...src/explore/components/controls/ViewQueryModal.tsx	`76.92% <66.66%> (+0.92%)`	⬆️
superset-frontend/src/components/Modal/Modal.tsx	`85.22% <73.46%> (-14.78%)`	⬇️
...et-frontend/src/components/Select/NativeSelect.tsx	`80.00% <0.00%> (-20.00%)`	⬇️
...frontend/src/views/CRUD/alert/AlertReportModal.tsx	`61.48% <0.00%> (-1.41%)`	⬇️
...views/CRUD/alert/components/NotificationMethod.tsx	`72.97% <0.00%> (-0.72%)`	⬇️
superset/db_engine_specs/presto.py	`89.95% <0.00%> (-0.42%)`	⬇️
...dashboard/components/SliceHeaderControls/index.tsx	`75.00% <0.00%> (-0.40%)`	⬇️
superset/views/base.py	`75.86% <0.00%> (-0.17%)`	⬇️
... and 41 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 c14364c...7866549. Read the comment docs.

geido · 2021-08-23T05:17:35Z

/testenv up

github-actions · 2021-08-23T05:19:37Z

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

junlincc · 2021-08-23T23:53:30Z

solid work!! lgtm! resizable, dragable, scroll as needed, awesome!! can you change the static modal in dashboard to the same? in a 2nd pr is fine

Screen.Recording.2021-08-23.at.6.49.23.PM.mov

Screen.Recording.2021-08-23.at.6.52.41.PM.mov

geido · 2021-08-24T08:53:44Z

Cool @junlincc. Let's get this merged first and I'll have a look at that Modal in the Dashboard

villebro

A few minor comments, but I'm sure these can be tackled later - LGTM and works really nicely! I also want to echo @junlincc request to enable this on the Dashboard view where I do the majority of query checking nowadays.

superset-frontend/src/components/Modal/Modal.tsx

villebro · 2021-08-24T11:38:30Z

superset-frontend/src/components/Modal/Modal.tsx

+  resizable?: boolean;
+  resizableConfig?: ResizableProps;
+  draggable?: boolean;
+  draggableConfig?: DraggableProps;


It feels slightly redundant to have four props for essentially two features. Would it be possible to squeeze the resizable into ResizableProps as an enabled prop which is assumed to default to false? Same for draggable.

I would say that resizableConfig and draggableConfig might be a bit overkilling here and the only reason why I put them there is that I cannot predict right now how many of those options we will need in the future. We might as well realize we don't need any and just shut them down.

So rather than squeezing resizable and draggable inside the config which looks like very standard props, I would first implement these types of Modals in a few places and then either shut the config props down altogether or just come up with a few elected ones as overridable defaults. What do you think?

Yes, that works just fine. The reason I pointed this out is this prop structure looked "unexpected" (for the lack of a better word), but I assumed there was a good reason for it. I completely agree; let's get this merged soon and do many small iterative improvements later rather than one big PR now 👍

junlincc

product sign-off🙏

@jinghua-qa let's add to test case.

github-actions · 2021-08-25T13:11:44Z

Ephemeral environment shutdown and build artifacts deleted.

* upstream/master: (25 commits) chore(ci): bump pylint to 2.10.2 (apache#16463) fix: prevent page crash when chart can't render (apache#16464) chore: fixed slack invite link (apache#16466) fix(native-filters): handle null values in value filter (apache#16460) feat: add function list to auto-complete to Clickhouse datasource (apache#16234) refactor(explore): improve typing for Dnd controls (apache#16362) fix(explore): update overwrite button on perm change (apache#16437) feat: Draggable and Resizable Modal (apache#16394) refactor: sql_json view endpoint (apache#16441) fix(dashboard): undo and redo buttons weird alignment (apache#16417) fix: setupPlugin in chart list page (apache#16413) fix: Disable Slack notification method if no api token (apache#16367) feat: add Shillelagh DB engine spec (apache#16416) fix: copy to Clipboard order (apache#16299) docs: make FEATURE_FLAGS.md reference a link (apache#16415) chore(viz): bump superset-ui to 0.17.87 (apache#16420) feat: add activate command (apache#16404) Revert "fix(explore): let admin overwrite slice (apache#16290)" (apache#16408) fix(explore): retain chart ownership on query context update (apache#16419) chore: Removes the TODOs and uses the default page size (apache#16422) ...

* Implement resizable prop * Implement resizableConfig * Implement fluid Syntax Highlighter * Implement draggable * Destroy on close * Implement draggableConfig * Enhance with footer calculation * Add new line * Make whole header draggable trigger

geido added 7 commits August 22, 2021 11:02

Implement resizable prop

30bc5e9

Implement resizableConfig

9455db6

Implement fluid Syntax Highlighter

3b7d096

Implement draggable

4ebc3fd

Destroy on close

8736365

Implement draggableConfig

c326538

Enhance with footer calculation

5877334

superset-github-bot bot added the Flexiana label Aug 22, 2021

pull-request-size bot added the size/L label Aug 22, 2021

geido added 2 commits August 23, 2021 00:24

Merge

2949e41

Add new line

3d5fcdf

geido requested review from rusackas, kgabryje, junlincc, michael-s-molina, suddjian and villebro August 22, 2021 21:26

junlincc added the need:review Requires a code review label Aug 23, 2021

Merge

f1f8906

villebro approved these changes Aug 24, 2021

View reviewed changes

Make whole header draggable trigger

7866549

junlincc approved these changes Aug 24, 2021

View reviewed changes

geido merged commit db11c3e into apache:master Aug 25, 2021

geido deleted the fix/issue_16205_draggable_resizable_modal branch August 25, 2021 13:12

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Draggable and Resizable Modal #16394

feat: Draggable and Resizable Modal #16394

geido commented Aug 22, 2021 •

edited

Loading

geido commented Aug 22, 2021

codecov bot commented Aug 22, 2021 •

edited

Loading

geido commented Aug 23, 2021

github-actions bot commented Aug 23, 2021

junlincc commented Aug 23, 2021

geido commented Aug 24, 2021

villebro left a comment

villebro Aug 24, 2021

geido Aug 24, 2021

villebro Aug 24, 2021

junlincc left a comment

github-actions bot commented Aug 25, 2021

feat: Draggable and Resizable Modal #16394

feat: Draggable and Resizable Modal #16394

Conversation

geido commented Aug 22, 2021 • edited Loading

SUMMARY

SHOULD I USE THESE OPTIONS?

FIRST EXAMPLE: VIEW QUERY MODAL

BEFORE

AFTER

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

geido commented Aug 22, 2021

codecov bot commented Aug 22, 2021 • edited Loading

Codecov Report

geido commented Aug 23, 2021

github-actions bot commented Aug 23, 2021

junlincc commented Aug 23, 2021

geido commented Aug 24, 2021

villebro left a comment

Choose a reason for hiding this comment

villebro Aug 24, 2021

Choose a reason for hiding this comment

geido Aug 24, 2021

Choose a reason for hiding this comment

villebro Aug 24, 2021

Choose a reason for hiding this comment

junlincc left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 25, 2021

geido commented Aug 22, 2021 •

edited

Loading

codecov bot commented Aug 22, 2021 •

edited

Loading