Split setup in advanced and regular for data sources and destinations #4160

gabrieldutra · 2019-09-20T21:13:21Z

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

Feature

Description

Following #3771 to simplify the setup for DS, this will allow selecting settings to be considered "advanced".

Related Tickets & Documents

#3771

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

Notes

I've randomly selected a few options to be considered advanced, just so we get the feel of it on Preview;
Default values for advanced options are filled in the form when you are creating a DS/Destination (regular options are turned into placeholders). This allows the user to fill in the form without even looking into the advanced options;
So far I'm using Antd's Collapse default for the job, but lmk if you have other suggestions 🙂

ranbena · 2019-09-21T05:46:25Z

Awesome @gabrieldutra.

A few thoughts:

I think the collapse style is good 👍
Give the collapsed element m-t-30, m-b-10 for some spacing.
I'd rename to "More options" since it's not necessarily "advanced".
Consider keeping this a front-end-only feature, by putting all non-required fields in the collapsed section, or by max field count.

Sidenote - I don't see a point to this extra padding:

redash/client/app/components/CreateSourceDialog.jsx

Line 103 in cb654b3

arikfr · 2019-09-21T09:11:11Z

@ranbena it can't be a front end only feature because each data source needs to pick which configuration should be at front and which should be collapsed. Naming it "More Options" makes sense. Also, is the collapsed state visible enough?

…

On Sat, Sep 21, 2019, 08:46 Ran Byron ***@***.***> wrote: Awesome @gabrieldutra <https://github.com/gabrieldutra>. A few thoughts: 1. I think the collapse style is good 👍 2. Give the collapsed element m-t-30, m-b-10 for some spacing. 3. I'd rename to "More options" since it's not necessarily "advanced". 4. Consider keeping this a front-end-only feature, by putting all non-required fields in the collapsed section, or by max field count. Sidenote - I don't see a point to this extra padding: https://github.com/getredash/redash/blob/cb654b3f2117507e2fcf2f866e8da5543665c928/client/app/components/CreateSourceDialog.jsx#L103 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4160?email_source=notifications&email_token=AAAROLBVVLRQP6Q7BCFDHOTQKWYLHA5CNFSM4IY3LCIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7ILCOQ#issuecomment-533770554>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAROLHPACQGGB2WTNECHUTQKWYLHANCNFSM4IY3LCIA> .

kravets-levko · 2019-09-21T09:27:02Z

We already have a bit customized Collapse in SaaS, maybe we can use it here as well?

gabrieldutra · 2019-09-21T13:00:58Z

Consider keeping this a front-end-only feature, by putting all non-required fields in the collapsed section

As Arik said, it's better to pick the fields that will be collapsed. There are cases where we want non-requierd fields to be visible (e.g: password field, it can be left empty, but it should be visible)

Also, is the collapsed state visible enough?

I was a bit concerned at the beginning, but it turned out to be ok to me. One other option is the SaaS style Levko mentioned.

Also, I think it makes sense to use the customized Collapse we have, so we keep consistent in the UI.

I've just realized the randomly selected options were done in the backend, so it's not possible to try this in preview 😅

ranbena · 2019-09-21T13:49:32Z

I made a kind of a hybrid between the two and I quite like it:

{
  height: 40px;
  font-weight: 500;
  background-color: @btn-danger-bg;
  border-color: @btn-danger-border;
}

arikfr · 2019-09-21T17:52:55Z

client/app/components/dynamic-form/dynamicFormHelper.js

+  Object.keys(properties).forEach((property) => {
+    if (!isUndefined(properties[property].default)) {
+      // set default value for checkboxes and for advanced options
+      if (properties[property].type === 'checkbox' || properties[property].advanced) {


Why the advanced options need special handling here?

This is related to the second note 🙂:

Default values for advanced options are filled in the form when you are creating a DS/Destination (regular options are turned into placeholders). This allows the user to fill in the form without even looking into the advanced options;

We show as a placeholder the default value and we use it as well. So there is no need to have a different behavior for the advanced options: they can skip them and the default value will still be used.

Although this made me think of why aren't we just filling in the default value for regular options as well? The only reason I can think of is to be able to change the default, but not sure if this is desired 🤔

Anyway, it's out of scope, so worth keeping the same behavior until we decide if we want to change.

I was thinking about required values also being able to be additional settings, but considering what you mentioned I shouldn't be worried about them at all (also not a very important context anyway)

A required setting will never be in advanced settings. The idea is to hide there all the things people won't really bother configure in normal cases. This doesn't apply to required settings.

ranbena

@gabrieldutra can you consolidate naming in codeadvanced options / extra options / additional settings?

gabrieldutra · 2019-09-21T18:46:08Z

consolidate naming in codeadvanced options / extra options / additional settings

Yes, that's on my plans as soon as I figure what makes more sense (it's been changing since I opened, so I haven't bother to do that yet)

ranbena · 2019-09-22T06:28:09Z

@arikfr @gabrieldutra I question the need to have the "Additional Settings" functionality in datasource/destination view mode. It ain't a modal so let it be as long as it's settings, no?

If not, I think at least we should have the additional settings open by default if a setting there has been modified.

arikfr · 2019-09-22T07:14:56Z

@arikfr @gabrieldutra I question the need to have the "Additional Settings" functionality in datasource/destination view mode. It ain't a modal so let it be as long as it's settings, no?

In general you're right. Let's address it when we actually have a view mode for Data Sources ;-)

ranbena · 2019-09-23T10:06:23Z

client/app/components/dynamic-form/DynamicForm.jsx

@@ -56,10 +56,11 @@ class DynamicForm extends React.Component {
  constructor(props) {
    super(props);

+    const filledExtraFields = filter(props.fields, field => field.extra && field.initialValue !== undefined);


Perhaps use some?

Notice that if a user sets an extra field but then clear the field - it won't be undefined but '' hence the section would open needlessly.

Maybe check !isEmpty(field.initialValue) instead?

It is a bit trickier to determine whether a field is empty 😕, undefined was the easiest I thought about (but there is the issue you mentioned). isEmptywon't work neither for number nor for booleans. I'm thinking about an alternative option as I would like to keep it as a DynamicForm behavior 😬

ranbena

Just a few comments on filledExtraFields.
Other than that, it's wonderful - great job!! 🤜 🤛

gabrieldutra · 2019-09-23T20:26:54Z

client/app/components/dynamic-form/DynamicForm.jsx

@@ -51,9 +56,14 @@ class DynamicForm extends React.Component {
  constructor(props) {
    super(props);

+    const hasFilledExtraField = some(props.fields, (field) => {
+      const { extra, initialValue } = field;
+      return extra && (!isEmpty(initialValue) || isNumber(initialValue) || isBoolean(initialValue) && initialValue);


@ranbena this seemed the simplest without separating this and deep evaluating the values with their defaults. I've also tried the last one and my opinion is that it's an unnecessary extra complexity.

gabrieldutra · 2019-09-23T21:13:24Z

@arikfr, I can go around and try to classify some DS/Destination fields as Extra Option (what of course should be reviewed afterwards), should I?

arikfr · 2019-09-24T10:03:49Z

@arikfr, I can go around and try to classify some DS/Destination fields as Extra Option (what of course should be reviewed afterwards), should I?

No need. We will do it in a separate PR.

arikfr · 2019-10-06T08:46:58Z

:-)

gabrieldutra added 3 commits September 20, 2019 17:58

DynamicForm support for advanced options

87ff1fa

Randomly select a few options to be advanced

42e86e9

Merge conditions with the same logic

6746fac

gabrieldutra self-assigned this Sep 20, 2019

Address some comments

82f1afa

Update styling for the button

70e563c

arikfr reviewed Sep 21, 2019

View reviewed changes

ranbena reviewed Sep 21, 2019

View reviewed changes

ranbena and others added 4 commits September 22, 2019 08:45

Some style adjustments (#4162)

df4d9cf

Don't set default value to additional settings

8a754d1

Rename advanced -> extra

085c53c

Show extra fields by default when they are filled

c152e21

ranbena reviewed Sep 23, 2019

View reviewed changes

ranbena approved these changes Sep 23, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Sep 23, 2019

Weekly Digest (16 September, 2019 - 23 September, 2019) #4172

Closed

gabrieldutra added 3 commits September 23, 2019 13:50

Merge branch 'master' into ds-advanced-options

51b4486

Update hasFilledExtraField logic

be2c4b0

Add example field from destination as extra

c415264

gabrieldutra commented Sep 23, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Sep 30, 2019

Weekly Digest (23 September, 2019 - 30 September, 2019) #4196

Closed

arikfr merged commit 8ea285d into master Oct 6, 2019

arikfr deleted the ds-advanced-options branch October 6, 2019 08:46

weekly-digest bot mentioned this pull request Oct 7, 2019

Weekly Digest (30 September, 2019 - 7 October, 2019) #4222

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split setup in advanced and regular for data sources and destinations #4160

Split setup in advanced and regular for data sources and destinations #4160

gabrieldutra commented Sep 20, 2019

ranbena commented Sep 21, 2019

arikfr commented Sep 21, 2019 via email

kravets-levko commented Sep 21, 2019

gabrieldutra commented Sep 21, 2019

ranbena commented Sep 21, 2019

arikfr Sep 21, 2019

gabrieldutra Sep 21, 2019

arikfr Sep 21, 2019

gabrieldutra Sep 21, 2019

arikfr Sep 21, 2019

ranbena left a comment

gabrieldutra commented Sep 21, 2019 •

edited

Loading

ranbena commented Sep 22, 2019

arikfr commented Sep 22, 2019

ranbena Sep 23, 2019

ranbena Sep 23, 2019

gabrieldutra Sep 23, 2019

ranbena left a comment

gabrieldutra Sep 23, 2019

gabrieldutra commented Sep 23, 2019 •

edited

Loading

arikfr commented Sep 24, 2019

arikfr commented Oct 6, 2019

Split setup in advanced and regular for data sources and destinations #4160

Split setup in advanced and regular for data sources and destinations #4160

Conversation

gabrieldutra commented Sep 20, 2019

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

Description

Related Tickets & Documents

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

Notes

ranbena commented Sep 21, 2019

arikfr commented Sep 21, 2019 via email

kravets-levko commented Sep 21, 2019

gabrieldutra commented Sep 21, 2019

ranbena commented Sep 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranbena left a comment

Choose a reason for hiding this comment

gabrieldutra commented Sep 21, 2019 • edited Loading

ranbena commented Sep 22, 2019

arikfr commented Sep 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranbena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra commented Sep 23, 2019 • edited Loading

arikfr commented Sep 24, 2019

arikfr commented Oct 6, 2019

gabrieldutra commented Sep 21, 2019 •

edited

Loading

gabrieldutra commented Sep 23, 2019 •

edited

Loading