Migrate Parameters component to React #4006

gabrieldutra · 2019-07-22T21:50:00Z

What type of PR is this? (check all applicable)

Refactor

Description

Migration of the Parameters Component to React.

Related Tickets & Documents

--

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

gabrieldutra · 2019-07-24T02:44:58Z

@ranbena it seems my worries became something to be figured out 😂, since parameters is an array and React does a shallow comparison it doesn't trigger rerender on the component. shouldComponentUpdate won't do it, it seems it exists only to improve performance (shouldComponentUpdate = () => true; by default).

I'm thinking about other options (had a workaround for the dragging feature so far). Need to either create a new array for changes or use an external update trigger. Can use an external angular $watch to help, but I don't want to rely on that... I'll think better about this tomorrow.

arikfr · 2019-07-24T05:29:09Z

Can use an external angular $watch to help, but I don't want to rely on that...

You definitely don't :-)

ranbena · 2019-07-24T08:10:52Z

@ranbena it seems my worries became something to be figured out 😂, since parameters is an array and React does a shallow comparison it doesn't trigger rerender on the component.

Why the need to re-render it?

gabrieldutra · 2019-07-24T19:45:54Z

Why the need to re-render it?

To the UI get updated when parameters get changed externally (e.g: parameter value changed).

ranbena · 2019-07-25T05:50:55Z

To the UI get updated when parameters get changed externally (e.g: parameter value changed).

You mean if someone else has modified the query / dashboard while another is viewing / editing it? I'm not sure that's a feature we should necessarily support.

gabrieldutra · 2019-07-25T16:22:46Z

You mean if someone else has modified the query / dashboard while another is viewing / editing it? I'm not sure that's a feature we should necessarily support.

Yep, I'm actually handling with state + an onUpdate function and will see where this goes. In any case, this is a bit trickier, there are cases where it's necessary to support (e.g: when you add a parameter in a query). So far, it's going well ^^.

BTW, I found a bug with the Apply Changes (initially thought it was an issue with this migration):

When you edit the query you lose the "Apply Changes" state. The cause for this is in here:

redash/client/app/services/query.js

Line 290 in c518c7a

    
           this.query.options.parameters = parameters.filter(parameterExists).map(p => new Parameter(p, this.query.id));

Parameter is recreated and the pendingValue is not initiated in the constructor.

I'll write a fix for it today :)

gabrieldutra · 2019-07-26T21:25:10Z

Yep, overall it seems to be working with the workaround I mentioned:

I'm using Antd Button as a Link instead of the button from the discussion (lmk if you prefer the button):

I'll officially open this for review tomorrow with Cypress tests + attempt to do the Drag Placeholder (just remembered of it)

ranbena · 2019-07-27T16:44:31Z

Seems to work well 👍
What's the sort save logic? (seems to be saved on "Save" click with no dirty indication)

I'm using Antd Button as a Link instead of the button from the discussion (lmk if you prefer the button):

I've weighed both options a lot while making the mockup till it drove me crazy! 🤪
I'd like to see the icon encased in a button, to get a sense of the look and feel. I'm still unsure about it. (Notice I went for a bootstrap button style cause ant ain't a good fit for this page yet)

attempt to do the Drag Placeholder (just remembered of it)

I think it's just a matter of giving .parameter-container { background-color: #f6f8f9; }.
But make sure to give each parameter proper padding.

(don't forget to take the opportunity to move the relevant css from ParameterValueInput.less)

gabrieldutra · 2019-07-27T17:49:32Z

What's the sort save logic? (seems to be saved on "Save" click with no dirty indication)

Exactly that, guess it should follow the save-query-if-not-dirty rule?

I've weighed both options a lot while making the mockup till it drove me crazy! 🤪
I'd like to see the icon encased in a button, to get a sense of the look and feel. I'm still unsure about it. (Notice I went for a bootstrap button style cause ant ain't a good fit for this page yet)

Cool, will update and we choose later.

I think it's just a matter of giving .parameter-container { background-color: #f6f8f9; }.

I initially thought it was easy like that, but this only works when the .parameter-container has only one row of parameters 😬

(don't forget to take the opportunity to move the relevant css from ParameterValueInput.less)

👍

ranbena · 2019-07-27T18:20:22Z

What's the sort save logic? (seems to be saved on "Save" click with no dirty indication)

Exactly that, guess it should follow the save-query-if-not-dirty rule?

Honestly I think any change to params (value, type, position) should be reflected with the "apply changes" button and not the "Save". Do you think you can swing that (just the position)?

I think it's just a matter of giving .parameter-container { background-color: #f6f8f9; }.

I initially thought it was easy like that, but this only works when the .parameter-container has only one row of parameters 😬

Try this:

.parameter-container {
  background-color: #f6f8f9;
  display: inline;
  padding: 35px 0 11px;
}
.parameter {
  display: inline-block;
  background: white;
  padding: 6px;
}

But perhaps styling the sortable ghost can work as well.

PS. Wdyt about lockToContainerEdges: true?

arikfr · 2019-07-27T19:08:35Z

Honestly I think any change to params (value, type, position) should be reflected with the "apply changes" button and not the "Save".

This doesn't feel right: "Apply Changes" is an end-user functionality that doesn't (necessarily) change the query object itself. Changing position, type, default value is a query editor functionality. Merging the two into "Apply Changes" feels confusing.

ranbena · 2019-07-27T19:17:31Z

This doesn't feel right: "Apply Changes" is an end-user functionality that doesn't (necessarily) change the query object itself. Changing position, type, default value is a query editor functionality. Merging the two into "Apply Changes" feels confusing.

I disagree. Save button is directly connected to the sql query content, while parameter content (apart for param key which appears in the sql query) has no representation in it, therefore no correlation. Save button for sql query, apply button for params. And they'll pay for the wall.

arikfr · 2019-07-28T10:06:04Z

Save button for sql query

This I can agree with, but I still don't think that "Apply Changes" is the right thing to use for applying parameter configuration changes.

I would retain the same behavior we have for other parameter configuration changes (i.e. auto save unless dirty) and revisit this when we redo this whole page.

…eact-parameters

gabrieldutra · 2019-08-05T02:00:39Z

client/cypress/support/commands.js

@@ -50,3 +50,18 @@ Cypress.Commands.add('fillInputs', (elements) => {
    cy.getByTestId(testId).clear().type(value);
  });
 });
+
+Cypress.Commands.add('dragBy', { prevSubject: true }, (subject, offsetLeft, offsetTop, force = false) => {


This is actually optional as I've already changed and eventually needed to create a new drag helper for Parameters. LMK if you prefer it as a Cypress command or a dashboard api function.

gabrieldutra · 2019-08-05T02:22:11Z

@ranbena (CC @arikfr) I again feel very happy with the result, I see 2 improvements within this migration: The updated Drag feature and the flexible width label naming. Overall I feel this is ready, will run some tests tomorrow to make sure it's solid, but code shouldn't change.

ranbena

I'm seeing some css misalignments.
I'll fix and send a PR soon.

ranbena

Fixed. Good to go 👍

gabrieldutra · 2019-08-05T21:15:39Z

client/app/components/ParameterValueInput.less

+  }
+
+  .@{ant-prefix}-select {
+    width: 100%;


Fixes this 🪲 (Select needs a min-width for search):

Select doesn't have minWidth 60px?

Yes, previously we were replacing that with the !important flag, so now it's flexible and has minWidth 60px

client/app/components/Parameters.less

Co-Authored-By: Ran Byron <ranbena@gmail.com>

gabrieldutra · 2019-08-07T01:19:44Z

@arikfr if you fine with it, this is good to go 🙂

ranbena · 2019-08-07T14:39:04Z

client/app/components/Parameters.jsx

@@ -138,7 +139,7 @@ export class Parameters extends React.Component {
        data-test={`ParameterName-${param.name}`}
      >
        <div className="parameter-heading">
-          <label>{param.title || param.name}</label>
+          <label>{param.title || toHuman(param.name)}</label>


gabrieldutra · 2019-08-07T14:59:08Z

Cypress making a good job: Looks like the new pending value set is not triggering Angular update, thus the query execute button doesn't change disabled status unless you change something else like the query text... I'm looking into that.

ranbena · 2019-08-14T08:50:00Z

@gabrieldutra let's land this!

(especially cause I want to test #4020 in Netlify Preview :P)

arikfr · 2019-08-14T08:54:27Z

Are you sure it's stable enough to land it a moment before cutting a release branch?

ranbena · 2019-08-14T08:56:29Z

Are you sure it's stable enough to land it a moment before cutting a release branch?

No. Better wait for it. Plz cut ✂️

arikfr · 2019-08-18T08:27:28Z

👍

* Migrate Parameters component to React (#4006) * Start Parameters Migration * Add dirtyCount * Use workaround with setState * Apply Changes * Add EditSettingsDialog * Add Cmd/Ctrl + Enter behavior * Remove isApplying * Delete Angular version of parameters * Update tests * Remove angular stuff * Update jest * Drag placeholder * Update events * Use old button styling and move css * Reviewing code * Add parameter rearrange test * Add Parameter Settings title change test * Update Parameter Settings button styling * Move parameter url logic back to Parameters * Disable url update when query is new * Styling changes (#4019) * Ran's title width styling * Update drag test * Improve sizing for Number inputs Co-Authored-By: Ran Byron <ranbena@gmail.com> * Fix issue with dragged parameter wrapping Co-Authored-By: Ran Byron <ranbena@gmail.com> * Don't reevaluate dirtyParamCount * Allow multiple values :) * Fix parameter alignments * Fix Select width on search * Update client/app/components/Parameters.less Co-Authored-By: Ran Byron <ranbena@gmail.com> * Humanize param.name * Make sure angular updates Execute disabled status * Add more flake8 tests and fail build if any test fails (#4055) * Add more flake8 tests and fail build if any test fails Run all flake8 E9xx + F63x + F7xx + F82x tests. * long = long in Python 2 * Fix: MySQL connections without SSL are failing (#4090) * Move connection logic into a single method & make sure not to pass ssl value if not used. * Remove wildcard import and format file. * Removed redash-newstyle.less (#4017) * Make sure we always pass a list to _get_column_lists (#4095) (some data sources might return None as the columns list) * [Data Sources] Add: Azure Data Explorer (Kusto) query runner (#4091) * [Data Sources] Add: Azure Data Explorer (Kusto) query runner * CodeClimate fixes * Remove TODO * Fixed configuration properties names for Azure Kusto * Azure Kusto: get_schema in one query * azure-kusto-data update to 0.0.32 * Add Kusto to the default query runners list * [Qubole] - Adding support to process Quantum query types. (#4066) * [Qubole] - Adding support to process Quantum query types. Quantum is a serverless interactive service that offers direct SQL access to user's data lake. Changes are made to accept `quantum` query type from user which makes `Cluster Label` as optional. * -Making quantum as defult query. -Dictionary safe access to connection parmeters * keeping pep8 standards * Maintainig pep8 std * Use latest version of qds-sdk * Use qds-sdk v1.13.0 * Use qds-sdk v1.12.0 * Use qds-sdk v1.13.0 * Updating SDK with verified version * hive as default query type * qds-sdk : Locking most recent release version * qds-sdk : Locking recent release version * falling back to original version of qds-sdk * Dashboard: when updating parameters, run only relevant queries (#3804) * refresh only affected queries in dashboard when parameters are changed * rename pendingParameters to updatedParameters * select which widgets to update according to their mapping as a dashboard-level parameter * use lodash's include * Migrate with SQL statements. (#4105) * Update badge in README.md to link to CircleCI (#4104) * Update README.md * Update README.md * Update README.md Co-Authored-By: Ran Byron <ranbena@gmail.com> * Update README.md * Fix widget bottom element alignment (#4110) * Fix: allow users with view only acces to use the queries in Query Results (#4112) * Fix: allow users with view only acces to access the queries * Add tests * Update error message * Update error message. Take 2 * Fix Dropdown parameter options appearing behind Dialog (#4109) * Add ability to use Ant's Table loading property when using ItemsTable (#4117) * Display data source icon in query editor (#4119) * Move annotation logic into Query Runner (#4113) * Code formatting * Move annotation logic into query runner, so it can be overriden in the query runner. * Add mixin to __all__ * Switch to flag instead of mixin * Feature (Redshift): option to set query group for adhoc/scheduled queries (#4114) * Add scheduled status to query job metadata. * Add: option to set query group for adhoc/scheduled Redshift queries * Scheduled might not be set for already enqueued queries. * Fix number param value normlization (#4116) * Use ng-src for data source icons (#4123) * hive_ds: show a user friendly error message when possible (#4121) * Update botocore, to get pass pip warning (#4122) * Widget table scroll-x visible (#4101) * Table viz horizontal scroll made visible * Added tests * Fixed snapshot pre-condition * Perhaps this would trigger percy * Upgrade Sentry-SDK and enable additional integratoins (#4127) * Update sentry-sdk version * Add additional Sentry integrations * Migrate Counter visualization to React (#4106) * Migrate Counter to React: Renderer * Migrate Counter to React: Editor * Cleanup * Review and fix rows indexing algorithm * Counter not properly scaled in editor * Fix wrong label for/input id pair * Tests * Tests * Fix vendor prefixes * Remove unnecessary useEffect dependencies * Update tests * Fix Percy snapshot names * Sync botocor eversions across requirements files. (#4128) * Decrease size of widget pagination (#4120) * Added tests * Perhaps this would trigger percy * Decrease size of widget pagination * Removed unused attr * Updated tests * Allow the user to decide how to handle null values in charts (#4071) * #2629 Refactor Chart visualization, add option for handling NULL values (keep/convert to 0.0) * Handle null values in line/area stacking code; some cleanup * Handle edge case: line/area stacking when last value of one of series is missing * Mjnor update to line/area stacking code * Fix line/area normalize to percents feature * Unit tests * Refine tests; add tests for prepareLayout function * Tests for prepareData (heatmap) function * Tests for prepareData (pie) function * Tests for prepareData (bar, line, area) function * Tests for prepareData (scatter, bubble) function * Tests for prepareData (box) function * Remove unused file * Alerts: Add more condition comparison options (#4134) * #4132 Add more condition comparison options * Add arguments to fallback lambda * Remove duplicate messages method (#4131) * Migrate Chart visualization to React Part 1: Renderer (#4130) * Migrate Chart visualization: Renderer * Refine PlotlyChart component; move stylesheets to visualization's folder * Migrate Custom JS Chart to React * Cleanup * Add jsconfig settings with '@' webpack alias (#4135) * Counter Editor: move components to own files (#4138) * Allow users to share aggregated usage information with us (#4108) * Initial commit of BeaconConsent component * Add comment about being able to change setting * Use <Text> correctly * Final version of consent screen * Show beacon consent message on homepage only if it wasn't enabled already. * Add consent setting to organization settings screen. * Add support for custom message in OrgSetting.save. * Implmenet consent saving. * If consent given, send extra data * Add HelpTrigger * Make CodeClimate happy * Wrap everything with DynamicComponent * CHANGELOG for V8-beta. (#4057) * CHANGELOG for V8-beta. * Update CHANGELOG.md * Update CHANGELOG.md * Update CHANGELOG.md * Bug fix: Query view doesn't sync parameters when selecting and deleting (#4146) * Query Snippets: Use onClick instead of link for 'Click here' option (#4144) * Snippets: Don't change url when not needed * Revert "Snippets: Don't change url when not needed" This reverts commit 2f346f3. * Query Snippets: use onClick instead of link * Color picker component (#4136) * Widget filters overlapped by visualization (#4137) * Fix: widget filters overlapped by visualization * Fix tests * Fix tests * CHANGELOG for v8.0.0-beta.2 (#4145) * Stop building tarballs. * Update version reference. * CHANGELOG for 8.0.0-beta.2

* Start Parameters Migration * Add dirtyCount * Use workaround with setState * Apply Changes * Add EditSettingsDialog * Add Cmd/Ctrl + Enter behavior * Remove isApplying * Delete Angular version of parameters * Update tests * Remove angular stuff * Update jest * Drag placeholder * Update events * Use old button styling and move css * Reviewing code * Add parameter rearrange test * Add Parameter Settings title change test * Update Parameter Settings button styling * Move parameter url logic back to Parameters * Disable url update when query is new * Styling changes (#4019) * Ran's title width styling * Update drag test * Improve sizing for Number inputs Co-Authored-By: Ran Byron <ranbena@gmail.com> * Fix issue with dragged parameter wrapping Co-Authored-By: Ran Byron <ranbena@gmail.com> * Don't reevaluate dirtyParamCount * Allow multiple values :) * Fix parameter alignments * Fix Select width on search * Update client/app/components/Parameters.less Co-Authored-By: Ran Byron <ranbena@gmail.com> * Humanize param.name * Make sure angular updates Execute disabled status

* Start Parameters Migration * Add dirtyCount * Use workaround with setState * Apply Changes * Add EditSettingsDialog * Add Cmd/Ctrl + Enter behavior * Remove isApplying * Delete Angular version of parameters * Update tests * Remove angular stuff * Update jest * Drag placeholder * Update events * Use old button styling and move css * Reviewing code * Add parameter rearrange test * Add Parameter Settings title change test * Update Parameter Settings button styling * Move parameter url logic back to Parameters * Disable url update when query is new * Styling changes (getredash#4019) * Ran's title width styling * Update drag test * Improve sizing for Number inputs Co-Authored-By: Ran Byron <ranbena@gmail.com> * Fix issue with dragged parameter wrapping Co-Authored-By: Ran Byron <ranbena@gmail.com> * Don't reevaluate dirtyParamCount * Allow multiple values :) * Fix parameter alignments * Fix Select width on search * Update client/app/components/Parameters.less Co-Authored-By: Ran Byron <ranbena@gmail.com> * Humanize param.name * Make sure angular updates Execute disabled status

gabrieldutra added 2 commits July 22, 2019 18:46

Start Parameters Migration

0408f28

Merge branch 'master' into react-parameters

3cdc232

gabrieldutra self-assigned this Jul 22, 2019

Add dirtyCount

15419f6

gabrieldutra added Frontend Frontend: React Frontend codebase migration to React labels Jul 24, 2019

Use workaround with setState

61541d4

Apply Changes

d9dbef5

gabrieldutra added 9 commits July 25, 2019 17:49

Add EditSettingsDialog

2fe9a16

Add Cmd/Ctrl + Enter behavior

1d51a66

Remove isApplying

0ede83a

Delete Angular version of parameters

cf49952

Update tests

f2ab142

Remove angular stuff

33db3bc

Merge branch 'master' into react-parameters

cdac798

Merge branch 'master' into react-parameters

5f81f3f

Update jest

3fb3e3f

gabrieldutra added 2 commits August 1, 2019 14:08

Merge branch 'react-parameters' of github.com:getredash/redash into r…

efc987f

…eact-parameters

Merge branch 'master' into react-parameters

8b7eec1

gabrieldutra commented Aug 5, 2019

View reviewed changes

Allow multiple values :)

cad5ff0

ranbena suggested changes Aug 5, 2019

View reviewed changes

Fix parameter alignments

542f650

ranbena approved these changes Aug 5, 2019

View reviewed changes

Fix Select width on search

f2be99b

gabrieldutra commented Aug 5, 2019

View reviewed changes

ranbena reviewed Aug 6, 2019

View reviewed changes

client/app/components/Parameters.less Outdated Show resolved Hide resolved

gabrieldutra and others added 2 commits August 6, 2019 14:31

Update client/app/components/Parameters.less

58b1b72

Co-Authored-By: Ran Byron <ranbena@gmail.com>

Merge branch 'master' into react-parameters

aa097e3

Humanize param.name

5d22e49

ranbena reviewed Aug 7, 2019

View reviewed changes

Make sure angular updates Execute disabled status

2a28a18

arikfr merged commit a1f11cb into master Aug 18, 2019

weekly-digest bot mentioned this pull request Aug 19, 2019

Weekly Digest (12 August, 2019 - 19 August, 2019) #4082

Closed

gabrieldutra mentioned this pull request Aug 22, 2019

Migrate Widget component to React #4020

Merged

1 task

ranbena mentioned this pull request Sep 28, 2019

Migrate Table visualization to React Part 2: Editor #4175

Merged

10 tasks

gabrieldutra deleted the react-parameters branch November 4, 2019 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Parameters component to React #4006

Migrate Parameters component to React #4006

gabrieldutra commented Jul 22, 2019 •

edited

Loading

gabrieldutra commented Jul 24, 2019 •

edited

Loading

arikfr commented Jul 24, 2019

ranbena commented Jul 24, 2019

gabrieldutra commented Jul 24, 2019

ranbena commented Jul 25, 2019

gabrieldutra commented Jul 25, 2019

gabrieldutra commented Jul 26, 2019 •

edited

Loading

ranbena commented Jul 27, 2019 •

edited

Loading

gabrieldutra commented Jul 27, 2019

ranbena commented Jul 27, 2019

arikfr commented Jul 27, 2019

ranbena commented Jul 27, 2019

arikfr commented Jul 28, 2019

gabrieldutra Aug 5, 2019

gabrieldutra commented Aug 5, 2019

ranbena left a comment

ranbena left a comment

gabrieldutra Aug 5, 2019 •

edited

Loading

ranbena Aug 6, 2019

gabrieldutra Aug 6, 2019

gabrieldutra commented Aug 7, 2019

ranbena Aug 7, 2019

gabrieldutra commented Aug 7, 2019 •

edited

Loading

ranbena commented Aug 14, 2019 •

edited

Loading

arikfr commented Aug 14, 2019

ranbena commented Aug 14, 2019

arikfr commented Aug 18, 2019

Migrate Parameters component to React #4006

Migrate Parameters component to React #4006

Conversation

gabrieldutra commented Jul 22, 2019 • edited Loading

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

gabrieldutra commented Jul 24, 2019 • edited Loading

arikfr commented Jul 24, 2019

ranbena commented Jul 24, 2019

gabrieldutra commented Jul 24, 2019

ranbena commented Jul 25, 2019

gabrieldutra commented Jul 25, 2019

gabrieldutra commented Jul 26, 2019 • edited Loading

ranbena commented Jul 27, 2019 • edited Loading

gabrieldutra commented Jul 27, 2019

ranbena commented Jul 27, 2019

arikfr commented Jul 27, 2019

ranbena commented Jul 27, 2019

arikfr commented Jul 28, 2019

gabrieldutra Aug 5, 2019

Choose a reason for hiding this comment

gabrieldutra commented Aug 5, 2019

ranbena left a comment

Choose a reason for hiding this comment

ranbena left a comment

Choose a reason for hiding this comment

gabrieldutra Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

ranbena Aug 6, 2019

Choose a reason for hiding this comment

gabrieldutra Aug 6, 2019

Choose a reason for hiding this comment

gabrieldutra commented Aug 7, 2019

ranbena Aug 7, 2019

Choose a reason for hiding this comment

gabrieldutra commented Aug 7, 2019 • edited Loading

ranbena commented Aug 14, 2019 • edited Loading

arikfr commented Aug 14, 2019

ranbena commented Aug 14, 2019

arikfr commented Aug 18, 2019

gabrieldutra commented Jul 22, 2019 •

edited

Loading

gabrieldutra commented Jul 24, 2019 •

edited

Loading

gabrieldutra commented Jul 26, 2019 •

edited

Loading

ranbena commented Jul 27, 2019 •

edited

Loading

gabrieldutra Aug 5, 2019 •

edited

Loading

gabrieldutra commented Aug 7, 2019 •

edited

Loading

ranbena commented Aug 14, 2019 •

edited

Loading