feat: keep placeholder on multiselect #11289

eschutho · 2020-10-15T16:48:54Z

SUMMARY

This fixes the revert of #11181: some visual elements broke from changes during the review. It would be great to add some visual testing in the future.. will look into that!
Also, it was pointed out that the user should be able to type in the field instead of just using the popup selector. Thanks for that feedback, so I changed this to a placeholder.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

New Story:

TEST PLAN

New jest unit tests. Storybook component added.

ADDITIONAL INFORMATION

codecov-io · 2020-10-15T17:14:30Z

Codecov Report

Merging #11289 (c35c0e8) into master (542d2e3) will decrease coverage by 3.44%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #11289      +/-   ##
==========================================
- Coverage   65.50%   62.06%   -3.45%     
==========================================
  Files         874      423     -451     
  Lines       42344    26325   -16019     
  Branches     3972        0    -3972     
==========================================
- Hits        27739    16339   -11400     
+ Misses      14476     9986    -4490     
+ Partials      129        0     -129

Flag	Coverage Δ
cypress	`?`
javascript	`?`
python	`62.06% <ø> (ø)`

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

Impacted Files	Coverage Δ
...-frontend/src/components/Button/Button.stories.jsx
superset-frontend/src/components/AsyncSelect.jsx
...t-frontend/src/dashboard/containers/SliceAdder.jsx
superset-frontend/src/components/FormRow.jsx
...perset-frontend/src/middleware/loggerMiddleware.js
...c/dashboard/components/dnd/AddSliceDragPreview.jsx
...end/src/dashboard/util/findFirstParentContainer.js
...d/src/dashboard/util/updateComponentParentsList.js
...src/explore/components/AdhocMetricStaticOption.jsx
...nd/src/explore/components/ExploreViewContainer.jsx
... and 437 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 542d2e3...9540af5. Read the comment docs.

eschutho · 2020-10-15T21:02:14Z

superset-frontend/src/explore/components/controls/MetricsControl.jsx

+          placeholder={
+            this.props.multi
+              ? t('choose one or more column or aggregate function')
+              : t('choose a column or aggregate function')


@junlincc to confirm, you're going to revisit this text at a later date, correct?

etr2460

a few comments, thanks for the re-PR!

etr2460 · 2020-10-16T02:30:36Z

superset-frontend/src/explore/components/controls/MetricsControl.jsx

-          placeholder={t('choose a column or aggregate function')}
+          placeholder={
+            this.props.multi
+              ? t('choose one or more column or aggregate function')


columns or aggregate functions?

I went back and forth on that too, and didn't even find anything definitive in the grammar handbooks, but I agree that the plural sounds more natural .

It doesn't seem like there IS a definitive answer... the rules conflict!

The Proximity Agreement says you should go with whichever is closer to the verb. In this case, that'd be "choose", so you would go with "column or agregate function".

However, the Notional Agreement seems to back us up here that these are cognitively a collection, so the plural treatment makes more sense.

"Don't take any notice of teachers and textbooks in such matters. Nor of logic. It is good to say 'More than one passenger was hurt,' although more than one equals at least two and therefore logically the verb ought to be plural were not singular was!"

–– CS Lewis

+1 for team plural. And yes, I'm going back to what I should be doing, now.

ha, that's great. I can also comment in the code that we went with the Notional Agreement here in case anyone wonders. jk :)

etr2460 · 2020-10-16T02:30:45Z

superset-frontend/src/explore/components/controls/AdhocFilterControl.jsx

@@ -267,7 +267,7 @@ export default class AdhocFilterControl extends React.Component {
        <OnPasteSelect
          isMulti
          name={`select-${this.props.name}`}
-          placeholder={t('choose a column or metric')}
+          placeholder={t('choose one or more column or metric')}


columns or metrics?

etr2460 · 2020-10-16T02:32:08Z

superset-frontend/src/components/Select/styles.tsx

+    };
+    placeholder: string;
+  };
+  cx: (a: string | null, b: any, c: string) => string | void;


i'd imagine we can pull cx, className, and getStylesattributes form a type in emotion or the superset styled library. Any thoughts @rusackas ?

Most of these attribute types are pulled in from the existing type on InputProps from react-select with the exception that I'm adding selectProps which exists on the component, but the library doesn't have it present in the types.. a bug on their part I think. Not sure how removing any attributes would break anything down the road.

Can you try if this works?

import { InputProps as ReactSelectInputProps, CommonProps } from 'react-select'; type InputProps = ReactSelectInputProps & { placeholder?: string; selectProps?: CommonProps<any>['selectProps']; }

Ah, those were exported. I missed them the first time around. thx. I am admittedly still leveling up my typescript chops, so this sort of worked, but I still had to declare some times for the selectProps properties below. Not sure why they didn't import correctly off the interface. It could be because I'm taking the placeholder of selectProps and assigning it to the placeholder on the input and maybe the types aren't the same, but if you have any suggestions, lmk!

eschutho · 2020-10-31T00:48:43Z

rebased to correct that failing cypress test. I think this should be ready for another look now.

ktmud · 2020-10-31T01:28:04Z

superset-frontend/src/components/Select/styles.tsx

+    }: {
+      isMulti?: boolean;
+      value?: ValueType<any>;
+      placeholder?: ReactNode;


I think the reason why you could't directly use selectProps below is because you've set it as an optional prop. Changing

type InputProps = ReactSelectInputProps & { selectProps?: SelectProps; }

to

type InputProps = ReactSelectInputProps & { selectProps: SelectProps; }

should do the trick. Sorry for my misleading example!

This did.. thanks! I just had one more issue with value.length that still needed to be asserted, but much better.

eschutho · 2020-11-03T05:00:00Z

rebase

eschutho · 2020-11-04T21:57:13Z

@ktmud I added in a story and also moved the styles to use getStyles

eschutho · 2020-11-04T22:08:36Z

added license header

eschutho · 2020-11-05T00:35:03Z

retry cypress

ktmud · 2020-11-09T20:59:13Z

For Groupby and Columns control, number of available options reduces after users selected columns as the same column cannot be selected twice. But the place holder still says x options where x is the initial number of options.

ktmud · 2020-11-09T21:03:32Z

Other than that this PR looks good to me. Thanks for the great work! Hope we can find some ways to re-enable the tests, but this now looks good at least from manual testing.

eschutho · 2020-11-09T22:01:46Z

For Groupby and Columns control, number of available options reduces after users selected columns as the same column cannot be selected twice. But the place holder still says x options where x is the initial number of options.

That's a great point. I'll look into that. Thx! Also, @suddjian and I were looking at the type assertion on value and made a small tweak to isMulti && Array.isArray(value) && value.length instead, and thought it read a little better. I made that update, so LMK any thoughts on that.

ktmud · 2020-11-09T22:51:41Z

made a small tweak to isMulti && Array.isArray(value) && value.length instead

Sounds good to me.

eschutho · 2020-11-10T01:20:38Z

@ktmud I'm now counting down the options, but question what to do with "Select All." Should it be counted as an option? What should it say when everything is selected except for "Select All"? I asked @junlincc here for her advise, but if anyone on your team has opinions, that would be welcome, too!

eschutho · 2020-11-10T01:23:51Z

superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts

@@ -33,14 +33,15 @@ describe('AdhocFilters', () => {

  let numScripts = 0;

-  it('Should load AceEditor scripts when needed', () => {
+  xit('Should load AceEditor scripts when needed', () => {


@kgabryje I was seeing errors on these in dev, fyi.

Can we do a follow up to re-enable these tests if it's not too much trouble?

yeah, I'll circle back on them.

eschutho · 2020-11-10T01:24:46Z

superset-frontend/package.json

@@ -190,7 +190,7 @@
    "@storybook/addons": "^5.3.19",
    "@storybook/client-api": "^6.0.21",
    "@storybook/preset-typescript": "^3.0.0",
-    "@storybook/react": "^6.0.13",
+    "@storybook/react": "^6.0.28",


bumping storybook resolved some issues happening in the last day or so:

-frontend/node_modules/react-docgen-typescript/lib/parser.js:496 var properties = initializer.properties; ^ TypeError: Cannot read property 'properties' of undefined

ktmud · 2020-11-10T02:32:05Z

I think we should always excluding "Select All" from counting and hide the placeholder when everything is selected.

junlincc · 2020-11-10T05:56:58Z

@eschutho thanks for looping me in. please feel free to work with Jesse directly. he is the expert in data viz and the explore control in Superset, and certainly is more knowledgeable than me when it comes to details. I am very comfortable having him making some product decisions in this area. @ktmud I hope you don't mind me deferring responsibilities to you. :)

eschutho · 2020-11-11T00:14:00Z

Thank you both for that feedback. I removed "select all" from the count. @ktmud my understanding of react-select is that it will calculate the options to show if isMulti and there is no filterOption method passed to the component by removing the value selected from the option list, but it doesn't not expose that value back to us. Because of this abstraction of the select options, I am calculating what is left by subtracting total selected values from total options. But in the case where we have a filterOption method that allows you to select an option twice, in the case of metrics or filters, this method won't work. But of course without evaluating the filterOption passed in, we won't know what it does. It seems to me that there aren't any filters that are using SelectControl that behaves in this way, but it still feels a bit brittle. Do you think this would work in this case or have other suggestions?

eschutho · 2020-11-11T00:15:32Z

Here's a video of the functionality now. I tested it with a few different chart types:

ktmud · 2020-11-11T00:28:28Z

I think your current approach works. The filterOption thing indeed is a drawback of react-select which we could potentially bypass in the future by creating a wrapper that handler filtering ourselves (I've been wanting to add match-sorter support for a while), but that certainly isn't within the scope of this PR.

eschutho · 2020-11-12T18:31:23Z

rebase, squash all and remove package-lock.json

ktmud · 2020-11-12T19:36:15Z

👏

eschutho · 2020-11-12T20:26:38Z

Thanks for all your help on this @ktmud!

ktmud · 2020-11-17T02:56:49Z

@eschutho I'm seeing sub par experience with FilterBox selects after this change:

Before

After

The inputs jump when focused
It triggers the browser's native autocomplete

eschutho · 2020-11-17T03:04:33Z

Thanks for notating that. I’ll see if I can reproduce tomorrow.

…

On Nov 16, 2020, at 6:57 PM, Jesse Yang ***@***.***> wrote: @eschutho I'm seeing sub par experience with FilterBox selects after this change: Before After The inputs jump when focused It triggers the browser's native autocomplete — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

This reverts commit b277f19.

superset-github-bot bot added the preset-io label Oct 15, 2020

pull-request-size bot added the size/L label Oct 15, 2020

eschutho force-pushed the elizabeth/placeholder-on-multiselect branch from 5d1c1f7 to a2f1f75 Compare October 15, 2020 16:58

ktmud changed the title ~~keep placeholder on multiselect~~ feat: keep placeholder on multiselect Oct 15, 2020

eschutho force-pushed the elizabeth/placeholder-on-multiselect branch from a2f1f75 to 9af6e57 Compare October 15, 2020 20:46

eschutho commented Oct 15, 2020

View reviewed changes

eschutho force-pushed the elizabeth/placeholder-on-multiselect branch from 9af6e57 to b4a3dcc Compare October 15, 2020 21:23

etr2460 reviewed Oct 16, 2020

View reviewed changes

eschutho force-pushed the elizabeth/placeholder-on-multiselect branch 3 times, most recently from 0705109 to fc7c370 Compare October 26, 2020 18:03

eschutho force-pushed the elizabeth/placeholder-on-multiselect branch 2 times, most recently from c99f642 to 748951d Compare October 31, 2020 00:41

ktmud reviewed Oct 31, 2020

View reviewed changes

eschutho force-pushed the elizabeth/placeholder-on-multiselect branch from b2b1339 to cd9d22e Compare November 3, 2020 04:59

eschutho force-pushed the elizabeth/placeholder-on-multiselect branch 2 times, most recently from 47b2355 to 6b68626 Compare November 4, 2020 02:26

eschutho marked this pull request as draft November 4, 2020 03:04

eschutho marked this pull request as ready for review November 4, 2020 21:57

eschutho force-pushed the elizabeth/placeholder-on-multiselect branch from 1be56a6 to ea0aa27 Compare November 4, 2020 22:08

eschutho closed this Nov 5, 2020

eschutho reopened this Nov 5, 2020

eschutho closed this Nov 5, 2020

eschutho reopened this Nov 5, 2020

eschutho force-pushed the elizabeth/placeholder-on-multiselect branch from 559b537 to dd1e43e Compare November 9, 2020 20:48

eschutho force-pushed the elizabeth/placeholder-on-multiselect branch from 79dd4d9 to 5235f81 Compare November 10, 2020 01:22

eschutho commented Nov 10, 2020

View reviewed changes

eschutho force-pushed the elizabeth/placeholder-on-multiselect branch from 619ee4c to c35c0e8 Compare November 12, 2020 18:17

keep placeholder on multiselect

9540af5

eschutho force-pushed the elizabeth/placeholder-on-multiselect branch from c35c0e8 to 9540af5 Compare November 12, 2020 18:30

ktmud merged commit b277f19 into apache:master Nov 12, 2020

eschutho deleted the elizabeth/placeholder-on-multiselect branch November 12, 2020 20:26

ktmud added a commit to ktmud/superset that referenced this pull request Nov 17, 2020

Revert "keep placeholder on multiselect (apache#11289)"

94abb29

This reverts commit b277f19.

eschutho mentioned this pull request Nov 18, 2020

fix: style multiselect input #11732

Merged

6 tasks

etr2460 pushed a commit to airbnb/superset-fork that referenced this pull request Nov 18, 2020

Revert "keep placeholder on multiselect (apache#11289)"

326bad4

This reverts commit b277f19.

auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020

keep placeholder on multiselect (apache#11289)

083a53f

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

feat: keep placeholder on multiselect #11289

feat: keep placeholder on multiselect #11289

Conversation

eschutho commented Oct 15, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-io commented Oct 15, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusackas Oct 21, 2020 • edited Loading

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

ktmud Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho commented Oct 31, 2020

ktmud Oct 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho commented Nov 3, 2020

eschutho commented Nov 4, 2020

eschutho commented Nov 4, 2020

eschutho commented Nov 5, 2020

ktmud commented Nov 9, 2020

ktmud commented Nov 9, 2020

eschutho commented Nov 9, 2020

ktmud commented Nov 9, 2020

eschutho commented Nov 10, 2020

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

ktmud commented Nov 10, 2020 • edited Loading

junlincc commented Nov 10, 2020

eschutho commented Nov 11, 2020 • edited Loading

eschutho commented Nov 11, 2020

ktmud commented Nov 11, 2020 • edited Loading

eschutho commented Nov 12, 2020 • edited Loading

ktmud commented Nov 12, 2020

eschutho commented Nov 12, 2020

ktmud commented Nov 17, 2020

Before

After

eschutho commented Nov 17, 2020 via email

eschutho commented Oct 15, 2020 •

edited

Loading

codecov-io commented Oct 15, 2020 •

edited

Loading

rusackas Oct 21, 2020 •

edited

Loading

ktmud Oct 22, 2020 •

edited

Loading

ktmud Oct 31, 2020 •

edited

Loading

ktmud commented Nov 10, 2020 •

edited

Loading

eschutho commented Nov 11, 2020 •

edited

Loading

ktmud commented Nov 11, 2020 •

edited

Loading

eschutho commented Nov 12, 2020 •

edited

Loading