Dashboard grid React migration #1 #3722

ranbena · 2019-04-18T11:31:01Z

Refactor

Description

Since gridstack has no React implementation, migrated to react-grid-layout.

Closes #3285.

Included

Not included (tests skipped)

Single column breakpoint (upcoming PR)
Widget auto-height (wip)

Changed

Only one resize handle (bottom-right).
Fixes remove widget in dashboard works only once #3202 (test allows opening menu after removal now unskipped)
Fixes Widgets obscured by "Add Widget" bar #3721.

Known Issues

~~Removing widget in preview mode, generates js error.~~
~~Dragging/Resizing but no change - still a saving indication is triggered~~
Widgets draggable beyond page as reported here. Leaving unattended.

ranbena · 2019-04-18T13:48:16Z

@gabrieldutra widget drag tests fail cause the placeholder now animates and also doesn't react as expected. Fiddling with it I realized there are 2 better ways to go about determining widget position delta:

Disable animations when testing.
Utilize the built-in retry mechanism of cyElement.should to wait until animation settles.
I like this better. This would require sth like this:

// original
it('moves one column when dragged over snap threshold', () => {
  dragBy(cy.get('@textboxEl'), 110).then((delta) => {
    expect(delta.left).to.eq(200);
  });
});

// suggested
it('moves one column when dragged over snap threshold', () => {
  const drag = () => cy.get('@textboxEl').dragBy(110);
  const getLeft = () => cy.get('@textboxEl').invoke('offset').its('left');

  cy.get('@textboxEl').should(($el) => {
    expect(drag).to.increase(getLeft).by(200);
  });
});

But, it's not possible cause for some reason Cypress doesn't expect increase() argument to be a function, only an object, although Chai API does allow it (introduced long ago).

What am I doing wrong?
Also, did we already talk about this? 🤔

gabrieldutra · 2019-04-18T14:50:42Z

What am I doing wrong?

Isn't this getLeft async? (as it uses cy.get) Perhaps if you turn this spec into a jQuery style by getting the element first cy.get().then(($el) => {...}). Then you should be able to run functions and assert with $el (there is a chance that the element in the variable doesn't get updated, if so, assertions won't work).

Also, did we already talk about this?

Kind of, I wanted before to use Chai with elements directly (it doesn't work at all), this is a bit different 🙂.

gabrieldutra · 2019-04-18T22:08:28Z

I've noticed this one while playing:

ranbena · 2019-04-19T11:37:40Z

I've noticed this one while playing:

10x @gabrieldutra, that's a grid feature I don't care for either.
Fixed in react-grid-layout/react-grid-layout#549 but not merged yet :/

It's possible we'll have to have our own fork, since the original repo is not very responsive.

kravets-levko

Looks really cool 👍 (and I'm so happy that we removed that ugly old grid code). I still need to try it, but right now there are some notes about the code.

client/app/components/dashboards/dashboard-grid.jsx

client/app/components/dashboards/lazyInjector.js

client/app/config/dashboard-grid-options.js

gabrieldutra

I played around a bit with the migrated features, didn't find any new issues 🙂.

+367 −727 LOC? 😮

BTW, Why dashboard-grid.jsx and not DashboardGrid.jsx?

ranbena · 2019-04-24T05:43:50Z

I played around a bit with the migrated features, didn't find any new issues 🙂.

10x!

+367 −727 LOC? 😮

There's still 2 upcoming PRs so don't believe it yet!

BTW, Why dashboard-grid.jsx and not DashboardGrid.jsx?

Right - I started work on this PR quite long ago before we had definitive standards.

arikfr · 2019-04-24T06:16:57Z

While trying it with the deploy preview Viz demo dashboard I noticed there is considerable lag when going into edit mode and when exiting edit mode (even when not doing any changes).

Any idea why is this?

ranbena · 2019-04-24T16:15:51Z

While trying it with the deploy preview Viz demo dashboard I noticed there is considerable lag when going into edit mode and when exiting edit mode (even when not doing any changes).

Any idea why is this?

@arikfr when clicking "Edit", all grid item's isDraggable and isResizable props are set to true, which renders each with a react-draggable wrapper and a react-resizable wrapper. It makes sense that the more grid items, the more time it would take to init them.

Tested these props with persistent false and lag disappeared.
Didn't find related open issue in repo.
I have no simple solution in mind.

One way is to introduce lazy loaded grid items according to viewport (using the Intersection Observer API).
@kravets-levko @gabrieldutra any suggestions?

Comments attended

kravets-levko

Since now I'm,responsible of merging this - it looks good for me. @arikfr, @ranbena, @gabrieldutra - if you have any notices/suggestions - please leave your comments. If everyone is happy with this code - I'll merge it.

arikfr · 2019-05-13T09:33:11Z

I just opened the last version on Netlify, and:

The delete ('X') button is always visible.
Considering you can only resize from the bottom-right corner, maybe the indicator there should be more prominent?

(2 is NTH, but 1 is a blocker of course)

arikfr · 2019-05-13T09:35:13Z

Also the performance on huge dashboards isn't great, but I guess there isn't much we can do aside from pushing forward with converting widgets and visualizations to React?

arikfr · 2019-05-13T09:44:25Z

Few more things:

The widget dropdown menu isn't seen if the widget is too short, see the text box here: https://redash-preview.netlify.com/dashboard/short-widget.
~~When the widget is too high, it overlaps with the bottom help box and you can't resize it. See the Albums widget on the dashboard above.~~
The trigger to open the widget menu is vertical on widgets and horizontal on text boxes. We should pick one :)

kravets-levko · 2019-05-13T09:45:05Z

@arikfr I tested it on our React migration dashboard (which I think is pretty large): it is a bit slower when moving/resizing widgets comparing to gridster-based variant, but not too slow. I think we should migrate everything to React first, and only then test it again to decide what to do with it. Issue with lag when entering/leaving editing mode is completely fixed.

For "X" button - yeah, don't know how did I miss that. Will fix now.

arikfr · 2019-05-13T09:48:26Z

Ignore 2 in my last comment -- it's actually fixed in this PR :)

…x widget actions menu

ranbena · 2019-05-13T14:33:21Z

@kravets-levko can you rebase with master?

kravets-levko · 2019-05-13T14:48:26Z

@ranbena Done

ranbena · 2019-05-13T16:41:13Z

@kravets-levko I thought it wasn't cause it's missing the grid markings #3656 but it's actually a bug.
I can work on it and push to this branch.

ranbena · 2019-05-13T23:13:39Z

@kravets-levko I pushed the fix. Tested on Chrome/FF/Safari on Mac. Tested public dashboard.
Added one change: markings now appear also when there are no widgets in dashboard yet 🤟

ranbena · 2019-05-13T23:30:20Z

@arikfr

Before	After

arikfr · 2019-05-16T12:43:59Z

Merged 🚀

* Dashboard grid React migration * Updated tests * Fixes comments * One col layout * Tests unskipped * Test fixes * Test fix * AutoHeight feature * Kebab-cased * Get rid of lazyInjector * Replace react-grid-layout with patched fork to fix performance issues * Fix issue with initial layout when page has a scrollbar * Decrease polling interval (500ms is too slow) * Rename file to match it's contents * Added some notes and very minor fixes * Fix Remove widget button (should be visible only in editing mode); fix widget actions menu * Fixed missing grid markings * Enhanced resize handle * Updated placeholder color * Render DashboardGrid only when dashboard is loaded

ranbena self-assigned this Apr 18, 2019

ranbena added Frontend: React Frontend codebase migration to React Feature: Dashboards labels Apr 18, 2019

ranbena added this to the Next milestone Apr 18, 2019

ranbena mentioned this pull request Apr 19, 2019

Refactored dashboard drag/resize testing #3726

Merged

1 task

ranbena force-pushed the 1-grid-migration branch from 379041d to 15246d3 Compare April 20, 2019 06:39

Dashboard grid React migration

1207dfb

ranbena force-pushed the 1-grid-migration branch from 15246d3 to 9ec2999 Compare April 22, 2019 13:25

Updated tests

c3bdcb6

ranbena force-pushed the 1-grid-migration branch from 9ec2999 to c3bdcb6 Compare April 23, 2019 05:22

ranbena marked this pull request as ready for review April 23, 2019 06:07

ranbena requested a review from kravets-levko April 23, 2019 06:07

kravets-levko previously requested changes Apr 23, 2019

View reviewed changes

client/app/components/dashboards/dashboard-grid.jsx Outdated Show resolved Hide resolved

client/app/components/dashboards/lazyInjector.js Outdated Show resolved Hide resolved

client/app/config/dashboard-grid-options.js Outdated Show resolved Hide resolved

ranbena requested a review from kravets-levko April 23, 2019 11:33

ranbena force-pushed the 1-grid-migration branch from 822d210 to cf14ce2 Compare April 23, 2019 11:46

Fixes comments

1bd12f8

ranbena force-pushed the 1-grid-migration branch from cf14ce2 to 1bd12f8 Compare April 23, 2019 12:13

gabrieldutra reviewed Apr 24, 2019

View reviewed changes

ranbena added 2 commits April 24, 2019 16:17

One col layout

736ebb7

Tests unskipped

138c026

Test fixes

9d9bb52

Test fix

e9c8e35

Rename file to match it's contents

ba6737f

weekly-digest bot mentioned this pull request May 6, 2019

Weekly Digest (29 April, 2019 - 6 May, 2019) #3763

Closed

Added some notes and very minor fixes

6851226

kravets-levko requested review from gabrieldutra and kravets-levko and removed request for gabrieldutra May 13, 2019 09:26

kravets-levko approved these changes May 13, 2019

View reviewed changes

Fix Remove widget button (should be visible only in editing mode); fi…

e9d1ffb

…x widget actions menu

Merge branch 'master' into 1-grid-migration

19bd052

Fixed missing grid markings

b16d6d3

ranbena added 2 commits May 13, 2019 21:05

Enhanced resize handle

2de07a6

Updated placeholder color

c0c278a

ranbena force-pushed the 1-grid-migration branch from d906909 to c0c278a Compare May 14, 2019 03:06

Render DashboardGrid only when dashboard is loaded

bfc7d4f

arikfr merged commit 606cf12 into master May 16, 2019

arikfr deleted the 1-grid-migration branch May 16, 2019 12:43

weekly-digest bot mentioned this pull request May 20, 2019

Weekly Digest (13 May, 2019 - 20 May, 2019) #3814

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashboard grid React migration #1 #3722

Dashboard grid React migration #1 #3722

ranbena commented Apr 18, 2019 •

edited

ranbena commented Apr 18, 2019 •

edited

gabrieldutra commented Apr 18, 2019

gabrieldutra commented Apr 18, 2019

ranbena commented Apr 19, 2019 •

edited

kravets-levko left a comment

gabrieldutra left a comment

ranbena commented Apr 24, 2019

arikfr commented Apr 24, 2019

ranbena commented Apr 24, 2019

kravets-levko left a comment

arikfr commented May 13, 2019

arikfr commented May 13, 2019

arikfr commented May 13, 2019 •

edited

kravets-levko commented May 13, 2019

arikfr commented May 13, 2019

ranbena commented May 13, 2019

kravets-levko commented May 13, 2019 •

edited

ranbena commented May 13, 2019

ranbena commented May 13, 2019

ranbena commented May 13, 2019

arikfr commented May 16, 2019

Dashboard grid React migration #1 #3722

Dashboard grid React migration #1 #3722

Conversation

ranbena commented Apr 18, 2019 • edited

Description

ranbena commented Apr 18, 2019 • edited

gabrieldutra commented Apr 18, 2019

gabrieldutra commented Apr 18, 2019

ranbena commented Apr 19, 2019 • edited

kravets-levko left a comment

Choose a reason for hiding this comment

gabrieldutra left a comment

Choose a reason for hiding this comment

ranbena commented Apr 24, 2019

arikfr commented Apr 24, 2019

ranbena commented Apr 24, 2019

kravets-levko left a comment

Choose a reason for hiding this comment

arikfr commented May 13, 2019

arikfr commented May 13, 2019

arikfr commented May 13, 2019 • edited

kravets-levko commented May 13, 2019

arikfr commented May 13, 2019

ranbena commented May 13, 2019

kravets-levko commented May 13, 2019 • edited

ranbena commented May 13, 2019

ranbena commented May 13, 2019

ranbena commented May 13, 2019

arikfr commented May 16, 2019

ranbena commented Apr 18, 2019 •

edited

ranbena commented Apr 18, 2019 •

edited

ranbena commented Apr 19, 2019 •

edited

arikfr commented May 13, 2019 •

edited

kravets-levko commented May 13, 2019 •

edited