You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2023/01/23 19:54:32 UTC
[GitHub] [superset] michael-s-molina opened a new pull request, #22830: fix: Handles disabled options on Select All
michael-s-molina opened a new pull request, #22830:
URL: https://github.com/apache/superset/pull/22830
### SUMMARY
Updates `Select` to correctly handle disabled options on Select All.
Follow-up of https://github.com/apache/superset/pull/22084
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
https://user-images.githubusercontent.com/70410625/214135879-d4502cc7-3e8a-4543-883d-4637b07d7ad0.mov
### TESTING INSTRUCTIONS
- Select All should count only disabled options that are selected initially.
- Select All should NOT count disabled options that are unselected.
- Select All shouldn't change the original state of disabled options.
- Clear All should preserve disabled options that are selected initially.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina merged pull request #22830: fix: Handles disabled options on Select All
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #22830:
URL: https://github.com/apache/superset/pull/22830
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #22830: fix: Handles disabled options on Select All
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #22830:
URL: https://github.com/apache/superset/pull/22830#issuecomment-1419533486
> @kgabryje Can you provide an example? I tested in the Storybook but was not able to reproduce the bug.
I think there's a problem with `Select`'s `onChange` function. When selecting/deselecting, `onChange` provides an array of all currently selected item ids. So when there is a disabled option that is selected by default, un-clicking `Select all` should call `onChange` with an array containing the disabled option's id. Instead, it's passing an empty array.
Mock piece of code that should allow to repro this example:
```
const [selectValue, setSelectValue] = useState(1);
const options = [{ value: 1, disabled: true }, { value: 2 }, { value: 3 }];
const handleChange = (selected) => setSelectValue(selected);
return <Select options={options} value={selectValue} onChange={handleChange} />
```
In this example, value `1` should be selected by default and be un-selectable. Un-selecting all will call `onChange` with an empty array and un-select value `1`.
Of course, I could work around it by adding logic to `handleChange` to ensure that `1` stays selected, but I think it'd be better to have that logic in `Select` component.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #22830: fix: Handles disabled options on Select All
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #22830:
URL: https://github.com/apache/superset/pull/22830#issuecomment-1408481306
Selecting all correctly ignores the disabled option, but un-selecting all does include the disabled option.
Example: We have options "A", "B" and "C". Option "A" is selected by default and disabled so that user can't unselect it. Clicking "Select all" correctly adds options "B" and "C" to selection, but clicking it again unselects all options, including disabled "A".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #22830: fix: Handles disabled options on Select All
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #22830:
URL: https://github.com/apache/superset/pull/22830#issuecomment-1422611389
> I think there's a problem with Select's onChange function.
@kgabryje There was a problem indeed! I submitted a fix for it in my last commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #22830: fix: Handles disabled options on Select All
Posted by codecov.
codecov[bot] commented on PR #22830:
URL: https://github.com/apache/superset/pull/22830#issuecomment-1400960994
# [Codecov](https://codecov.io/gh/apache/superset/pull/22830?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22830](https://codecov.io/gh/apache/superset/pull/22830?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (882439b) into [master](https://codecov.io/gh/apache/superset/commit/d479009e35a86dfda321492afeda2a1683a9345a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d479009) will **increase** coverage by `0.00%`.
> The diff coverage is `77.77%`.
```diff
@@ Coverage Diff @@
## master #22830 +/- ##
=======================================
Coverage 67.28% 67.28%
=======================================
Files 1877 1877
Lines 72019 72028 +9
Branches 7897 7898 +1
=======================================
+ Hits 48458 48466 +8
Misses 21533 21533
- Partials 2028 2029 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `53.86% <77.77%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/22830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `90.36% <77.77%> (-0.09%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/22830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `45.08% <0.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #22830: fix: Handles disabled options on Select All
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #22830:
URL: https://github.com/apache/superset/pull/22830#issuecomment-1412203730
> Selecting all correctly ignores the disabled option, but un-selecting all does include the disabled option. Example: We have options "A", "B" and "C". Option "A" is selected by default and disabled so that user can't unselect it. Clicking "Select all" correctly adds options "B" and "C" to selection, but clicking it again unselects all options, including disabled "A".
@kgabryje Can you provide an example? I tested in the Storybook but was not able to reproduce the bug.
https://user-images.githubusercontent.com/70410625/216078994-52d2fc12-5eb6-4605-82aa-8bef4fed371e.mov
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org