You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/07/12 20:15:52 UTC
[GitHub] [superset] cccs-RyanK opened a new pull request, #20690: Remove unecessary code from async and sync select components
cccs-RyanK opened a new pull request, #20690:
URL: https://github.com/apache/superset/pull/20690
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This task was outlined as a part of this PR by @michael-s-molina : https://github.com/apache/superset/pull/20143
In a previous PR, the code for Select and AsyncSelect was split into two separate components: https://github.com/apache/superset/pull/20466
The components were left identical, so this PR continues the work and removes the unecessary code that was left in each of the components. Namely, it removes behaviour that had previously been determined by whether the component was in async mode or not.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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 commented on a diff in pull request #20690: chore: Remove unecessary code from async and sync select components
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20690:
URL: https://github.com/apache/superset/pull/20690#discussion_r922168290
##########
superset-frontend/src/components/Select/AsyncSelect.tsx:
##########
@@ -299,7 +298,7 @@ const AsyncSelect = (
filterOption = true,
header = null,
invertSelection = false,
- labelInValue = false,
+ labelInValue = true,
Review Comment:
Can you remove the `labelInValue` prop from here and `PickedSelectProps`?
##########
superset-frontend/src/components/Select/AsyncSelect.tsx:
##########
@@ -322,9 +321,8 @@ const AsyncSelect = (
}: AsyncSelectProps,
ref: RefObject<AsyncSelectRef>,
) => {
- const isAsync = typeof options === 'function';
const isSingleMode = mode === 'single';
- const shouldShowSearch = isAsync || allowNewOptions ? true : showSearch;
+ const shouldShowSearch = allowNewOptions ? true : showSearch;
Review Comment:
We can remove `shouldShowSearch` and use only `showSearch` because the previous logic ignored the `allowNewOptions` if it was async. `showSearch` is true by default.
##########
superset-frontend/src/components/Select/AsyncSelect.tsx:
##########
@@ -706,13 +692,13 @@ const AsyncSelect = (
getPopupContainer={
getPopupContainer || (triggerNode => triggerNode.parentNode)
}
- labelInValue={isAsync || labelInValue}
+ labelInValue={labelInValue}
Review Comment:
```suggestion
labelInValue
```
--
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 #20690: chore: Remove unecessary code from async and sync select components
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20690:
URL: https://github.com/apache/superset/pull/20690#issuecomment-1191731457
# [Codecov](https://codecov.io/gh/apache/superset/pull/20690?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 [#20690](https://codecov.io/gh/apache/superset/pull/20690?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7a6cf25) into [master](https://codecov.io/gh/apache/superset/commit/922b4b8d1dd6767d9e675ce95b3ffefe16034a7a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (922b4b8) will **increase** coverage by `0.03%`.
> The diff coverage is `77.77%`.
```diff
@@ Coverage Diff @@
## master #20690 +/- ##
==========================================
+ Coverage 66.29% 66.33% +0.03%
==========================================
Files 1758 1758
Lines 66801 66703 -98
Branches 7055 7025 -30
==========================================
- Hits 44288 44249 -39
+ Misses 20713 20660 -53
+ Partials 1800 1794 -6
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.99% <77.77%> (+0.03%)` | :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/20690?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...frontend/src/components/DatabaseSelector/index.tsx](https://codecov.io/gh/apache/superset/pull/20690/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRGF0YWJhc2VTZWxlY3Rvci9pbmRleC50c3g=) | `88.88% <ø> (ø)` | |
| [...et-frontend/src/components/TableSelector/index.tsx](https://codecov.io/gh/apache/superset/pull/20690/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVGFibGVTZWxlY3Rvci9pbmRleC50c3g=) | `76.08% <ø> (ø)` | |
| [...t-frontend/src/filters/components/GroupBy/types.ts](https://codecov.io/gh/apache/superset/pull/20690/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9Hcm91cEJ5L3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
| [...rontend/src/filters/components/TimeColumn/types.ts](https://codecov.io/gh/apache/superset/pull/20690/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9UaW1lQ29sdW1uL3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
| [...frontend/src/filters/components/TimeGrain/types.ts](https://codecov.io/gh/apache/superset/pull/20690/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9UaW1lR3JhaW4vdHlwZXMudHM=) | `100.00% <ø> (ø)` | |
| [...rontend/src/components/ListView/Filters/Select.tsx](https://codecov.io/gh/apache/superset/pull/20690/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvRmlsdGVycy9TZWxlY3QudHN4) | `52.38% <33.33%> (-2.62%)` | :arrow_down: |
| [...set-frontend/src/components/Select/AsyncSelect.tsx](https://codecov.io/gh/apache/superset/pull/20690/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L0FzeW5jU2VsZWN0LnRzeA==) | `83.18% <80.00%> (+0.34%)` | :arrow_up: |
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/20690/diff?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=) | `87.07% <100.00%> (+19.29%)` | :arrow_up: |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?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 a diff in pull request #20690: chore: Remove unecessary code from async and sync select components
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20690:
URL: https://github.com/apache/superset/pull/20690#discussion_r920332830
##########
superset-frontend/src/components/Select/Select.tsx:
##########
@@ -322,25 +286,19 @@ const Select = (
}: SelectProps,
ref: RefObject<SelectRef>,
) => {
- const isAsync = typeof options === 'function';
const isSingleMode = mode === 'single';
- const shouldShowSearch = isAsync || allowNewOptions ? true : showSearch;
+ const shouldShowSearch = allowNewOptions ? true : showSearch;
const [selectValue, setSelectValue] = useState(value);
const [inputValue, setInputValue] = useState('');
const [isLoading, setIsLoading] = useState(loading);
const [error, setError] = useState('');
const [isDropdownVisible, setIsDropdownVisible] = useState(false);
- const [page, setPage] = useState(0);
- const [totalCount, setTotalCount] = useState(0);
- const [loadingEnabled, setLoadingEnabled] = useState(!lazyLoading);
- const [allValuesLoaded, setAllValuesLoaded] = useState(false);
const fetchedQueries = useRef(new Map<string, number>());
Review Comment:
You can remove `fetchedQueries` and all its references.
##########
superset-frontend/src/components/Select/Select.tsx:
##########
@@ -148,13 +126,6 @@ export interface SelectProps extends PickedSelectProps {
* False by default.
*/
invertSelection?: boolean;
- /**
- * It fires a request against the server only after
- * searching.
- * Works in async mode only (See the options property).
- * Undefined by default.
- */
- fetchOnlyOnSearch?: boolean;
Review Comment:
Can you remove `onError` prop and the associated error component, `internalOnError`, and error state?
##########
superset-frontend/src/components/Select/Select.tsx:
##########
@@ -358,10 +316,9 @@ const Select = (
const sortComparatorForNoSearch = useCallback(
Review Comment:
You can remove `sortComparatorForNoSearch` and just use `sortSelectedFirst` now that the code does not have another comparison.
##########
superset-frontend/src/components/Select/Select.tsx:
##########
@@ -77,12 +74,6 @@ export type OptionsTypePage = {
totalCount: number;
};
-export type OptionsPagePromise = (
- search: string,
- page: number,
- pageSize: number,
-) => Promise<OptionsTypePage>;
-
export type SelectRef = HTMLInputElement & { clearCache: () => void };
Review Comment:
We should remove the `SelectRef` as well because there's no more cache. Just use `HTMLInputElement` as the ref type.
We should also remove `clearCache` and `useImperativeHandle` below.
##########
superset-frontend/src/components/Select/Select.tsx:
##########
@@ -133,13 +117,7 @@ export interface SelectProps extends PickedSelectProps {
* The options can also be async, a promise that returns
* an array of options.
*/
- options: OptionsType | OptionsPagePromise;
Review Comment:
Can you remove `OptionsTypePage` declaration as well?
##########
superset-frontend/src/components/Select/Select.tsx:
##########
@@ -358,10 +316,9 @@ const Select = (
const sortComparatorForNoSearch = useCallback(
(a: AntdLabeledValue, b: AntdLabeledValue) =>
sortSelectedFirst(a, b) ||
- // Only apply the custom sorter in async mode because we should
- // preserve the options order as much as possible.
- (isAsync ? sortComparator(a, b, '') : 0),
- [isAsync, sortComparator, sortSelectedFirst],
+ // we should preserve the options order as much as possible.
+ 0,
+ [sortComparator, sortSelectedFirst],
);
const initialOptions = useMemo(
Review Comment:
We should remove `internalOnError` below.
--
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 a diff in pull request #20690: chore: Remove unecessary code from async and sync select components
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20690:
URL: https://github.com/apache/superset/pull/20690#discussion_r927026923
##########
superset-frontend/src/components/Select/AsyncSelect.tsx:
##########
@@ -706,20 +689,20 @@ const AsyncSelect = (
getPopupContainer={
getPopupContainer || (triggerNode => triggerNode.parentNode)
}
- labelInValue={isAsync || labelInValue}
+ labelInValue
maxTagCount={MAX_TAG_COUNT}
mode={mappedMode}
notFoundContent={isLoading ? t('Loading...') : notFoundContent}
onDeselect={handleOnDeselect}
onDropdownVisibleChange={handleOnDropdownVisibleChange}
- onPopupScroll={isAsync ? handlePagination : undefined}
- onSearch={shouldShowSearch ? handleOnSearch : undefined}
+ onPopupScroll={handlePagination}
+ onSearch={showSearch ? handleOnSearch : undefined}
onSelect={handleOnSelect}
onClear={handleClear}
onChange={onChange}
options={hasCustomLabels ? undefined : fullSelectOptions}
placeholder={placeholder}
- showSearch={shouldShowSearch}
+ showSearch
Review Comment:
```suggestion
showSearch={showSearch}
```
--
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 #20690: chore: Remove unecessary code from async and sync select components
Posted by GitBox <gi...@apache.org>.
michael-s-molina merged PR #20690:
URL: https://github.com/apache/superset/pull/20690
--
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 #20690: chore: Remove unecessary code from async and sync select components
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20690:
URL: https://github.com/apache/superset/pull/20690#issuecomment-1183520096
@cccs-RyanK About your question on Slack... I think it's a good idea to clean the components first and extract shared behaviors in another PR. It will be easier to check what can be extracted after the cleanup and it will be easier to review the changes.
--
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 a diff in pull request #20690: chore: Remove unecessary code from async and sync select components
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20690:
URL: https://github.com/apache/superset/pull/20690#discussion_r927053488
##########
superset-frontend/src/components/Select/AsyncSelect.tsx:
##########
@@ -706,20 +689,20 @@ const AsyncSelect = (
getPopupContainer={
getPopupContainer || (triggerNode => triggerNode.parentNode)
}
- labelInValue={isAsync || labelInValue}
+ labelInValue
maxTagCount={MAX_TAG_COUNT}
mode={mappedMode}
notFoundContent={isLoading ? t('Loading...') : notFoundContent}
onDeselect={handleOnDeselect}
onDropdownVisibleChange={handleOnDropdownVisibleChange}
- onPopupScroll={isAsync ? handlePagination : undefined}
- onSearch={shouldShowSearch ? handleOnSearch : undefined}
+ onPopupScroll={handlePagination}
+ onSearch={showSearch ? handleOnSearch : undefined}
onSelect={handleOnSelect}
onClear={handleClear}
onChange={onChange}
options={hasCustomLabels ? undefined : fullSelectOptions}
placeholder={placeholder}
- showSearch={shouldShowSearch}
+ showSearch
Review Comment:
Can you also make sure to memoize `initialValue` in the Storybook? Otherwise, if you set it to true in Storybook we'll end up with an infinite loop. When the story re-renders due to a request, it's passing a new `value` object every time which in turn will fire a request again and so on.
```
const initialValue = useMemo(
() => ({ label: 'Valentina', value: 'Valentina' }),
[],
);
<AsyncSelect
...
value={withInitialValue ? initialValue : undefined}
/>
```
--
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 a diff in pull request #20690: chore: Remove unecessary code from async and sync select components
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20690:
URL: https://github.com/apache/superset/pull/20690#discussion_r927053488
##########
superset-frontend/src/components/Select/AsyncSelect.tsx:
##########
@@ -706,20 +689,20 @@ const AsyncSelect = (
getPopupContainer={
getPopupContainer || (triggerNode => triggerNode.parentNode)
}
- labelInValue={isAsync || labelInValue}
+ labelInValue
maxTagCount={MAX_TAG_COUNT}
mode={mappedMode}
notFoundContent={isLoading ? t('Loading...') : notFoundContent}
onDeselect={handleOnDeselect}
onDropdownVisibleChange={handleOnDropdownVisibleChange}
- onPopupScroll={isAsync ? handlePagination : undefined}
- onSearch={shouldShowSearch ? handleOnSearch : undefined}
+ onPopupScroll={handlePagination}
+ onSearch={showSearch ? handleOnSearch : undefined}
onSelect={handleOnSelect}
onClear={handleClear}
onChange={onChange}
options={hasCustomLabels ? undefined : fullSelectOptions}
placeholder={placeholder}
- showSearch={shouldShowSearch}
+ showSearch
Review Comment:
Can you also make sure to memoize `initialValue` in the Storybook? Otherwise, if you set it to true in Storybook we'll end up with an infinite loop. When the story re-renders due to a request, it's passing a new value object every time which in turn will fire a request again and so on.
```
const initialValue = useMemo(
() => ({ label: 'Valentina', value: 'Valentina' }),
[],
);
<AsyncSelect
...
value={withInitialValue ? initialValue : undefined}
/>
```
--
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