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/10/29 22:14:30 UTC
[GitHub] [superset] michael-s-molina opened a new pull request, #21974: feat: Adds the DropdownContainer component
michael-s-molina opened a new pull request, #21974:
URL: https://github.com/apache/superset/pull/21974
### SUMMARY
Adds a new component called `DropdownContainer`. The component is a horizontal container that displays overflowed items in a popover. It also displays an indicator of how many items are currently overflowing.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
https://user-images.githubusercontent.com/70410625/198854261-985c2419-7458-47f2-b643-57206f6bbbd4.mov
### TESTING INSTRUCTIONS
Open the Storybook and check the new component.
### 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 commented on a diff in pull request #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21974:
URL: https://github.com/apache/superset/pull/21974#discussion_r1009651149
##########
superset-frontend/src/components/DropdownContainer/index.tsx:
##########
@@ -0,0 +1,233 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { CSSProperties, useEffect, useMemo, useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { useResizeDetector } from 'react-resize-detector';
+import { usePrevious } from 'src/hooks/usePrevious';
+import Badge from '../Badge';
+import Icons from '../Icons';
+import Button from '../Button';
+import Popover from '../Popover';
+
+/**
+ * Horizontal container that displays overflowed items in a popover.
+ * It shows an indicator of how many items are currently overflowed.
+ */
+export interface DropdownContainerProps {
+ /**
+ * Array of items. The id property is used to uniquely identify
+ * the elements when rendering or dealing with event handlers.
+ */
+ items: {
+ id: string;
+ element: React.ReactElement;
+ }[];
+ /**
+ * Event handler called every time an element moves between
+ * main container and popover.
+ */
+ onOverflowingStateChange?: (overflowingState: {
+ notOverflowed: string[];
+ overflowed: string[];
+ }) => void;
+ /**
+ * Popover additional style properties.
+ */
+ popoverStyle?: CSSProperties;
+ /**
+ * Icon of the popover trigger.
+ */
+ popoverTriggerIcon?: React.ReactElement;
+ /**
+ * Text of the popover trigger.
+ */
+ popoverTriggerText?: string;
+ /**
+ * Main container additional style properties.
+ */
+ style?: CSSProperties;
+}
+
+const DropdownContainer = ({
+ items,
+ onOverflowingStateChange,
+ popoverStyle = {},
+ popoverTriggerIcon,
+ popoverTriggerText = t('More'),
+ style,
+}: DropdownContainerProps) => {
+ const theme = useTheme();
+ const { ref, width = 0 } = useResizeDetector<HTMLDivElement>();
+ const previousWidth = usePrevious(width) || 0;
+ const { current } = ref;
+ const [overflowingIndex, setOverflowingIndex] = useState<number>(-1);
+ const [itemsWidth, setItemsWidth] = useState<number[]>([]);
+
+ useEffect(() => {
+ const container = current?.children.item(0);
+ if (container) {
+ const { children } = container;
+ const childrenArray = Array.from(children);
+
+ // Stores items width once
+ if (itemsWidth.length === 0) {
+ setItemsWidth(
+ childrenArray.map(child => child.getBoundingClientRect().width),
+ );
+ }
+
+ // Calculates the index of the first overflowed element
+ const index = childrenArray.findIndex(
+ child =>
+ child.getBoundingClientRect().right >
+ container.getBoundingClientRect().right,
+ );
+ setOverflowingIndex(index === -1 ? children.length : index);
+
+ if (width > previousWidth && overflowingIndex !== -1) {
+ // Calculates remaining space in the container
+ const button = current?.children.item(1);
+ const buttonRight = button?.getBoundingClientRect().right || 0;
+ const containerRight = current?.getBoundingClientRect().right || 0;
+ const remainingSpace = containerRight - buttonRight;
+ // Checks if the first element in the popover fits in the remaining space
+ const fitsInRemainingSpace = remainingSpace >= itemsWidth[0];
+ if (fitsInRemainingSpace && overflowingIndex < items.length) {
+ // Moves element from popover to container
+ setOverflowingIndex(overflowingIndex + 1);
+ }
+ }
+ }
+ }, [
+ current,
+ items.length,
+ itemsWidth,
+ overflowingIndex,
+ previousWidth,
+ width,
+ ]);
+
+ const notOverflowedItems = useMemo(
+ () =>
+ items
+ .slice(0, overflowingIndex !== -1 ? overflowingIndex : items.length)
+ .map(item => React.cloneElement(item.element, { key: item.id })),
+ [items, overflowingIndex],
+ );
+
+ const notOverflowedIds = useMemo(
+ () =>
+ items
+ .slice(0, overflowingIndex !== -1 ? overflowingIndex : items.length)
+ .map(item => item.id),
+ [items, overflowingIndex],
+ );
+
+ const overflowedItems = useMemo(
+ () =>
+ overflowingIndex !== -1
+ ? items
+ .slice(overflowingIndex, items.length)
+ .map(item => React.cloneElement(item.element, { key: item.id }))
+ : [],
+ [items, overflowingIndex],
+ );
+
+ const overflowedIds = useMemo(
+ () =>
+ overflowingIndex !== -1
+ ? items.slice(overflowingIndex, items.length).map(item => item.id)
+ : [],
+ [items, overflowingIndex],
+ );
+
+ useEffect(() => {
+ if (onOverflowingStateChange) {
+ onOverflowingStateChange({
+ notOverflowed: notOverflowedIds,
+ overflowed: overflowedIds,
+ });
+ }
+ }, [notOverflowedIds, onOverflowingStateChange, overflowedIds]);
+
+ const popoverContent = useMemo(
+ () => (
+ <div
+ css={css`
+ display: flex;
+ flex-direction: column;
+ gap: ${theme.gridUnit * 3}px;
+ width: 200px;
+ `}
+ style={popoverStyle}
+ >
+ {overflowedItems}
Review Comment:
I added the `popoverContent` property ππΌ
--
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 #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #21974:
URL: https://github.com/apache/superset/pull/21974#issuecomment-1297395968
> in the recording we can see some flashing of appearing/disappearing elements during resizing. I'm assuming that happens when container size is on the verge of fitting or not an additional element. Could we maybe add a few pixels hysteresis to changing the state of displayed/hidden elements?
Actually, this is somewhat related to Select's horizontal label. If you set the label position at the top or remove it, you'll see it's no longer flicking. I'll check this alongside the RTL tests. I just wanted to resolve the blocking ones first in case we need to merge the PR to unblock other things.
--
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 #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21974:
URL: https://github.com/apache/superset/pull/21974#issuecomment-1297481294
# [Codecov](https://codecov.io/gh/apache/superset/pull/21974?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 [#21974](https://codecov.io/gh/apache/superset/pull/21974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d3f6349) into [master](https://codecov.io/gh/apache/superset/commit/779d9f75336ce38ab346e27dcb6a77e5a68cf823?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (779d9f7) will **decrease** coverage by `0.04%`.
> The diff coverage is `17.18%`.
```diff
@@ Coverage Diff @@
## master #21974 +/- ##
==========================================
- Coverage 66.95% 66.91% -0.05%
==========================================
Files 1807 1808 +1
Lines 69196 69282 +86
Branches 7402 7432 +30
==========================================
+ Hits 46331 46357 +26
- Misses 20954 21012 +58
- Partials 1911 1913 +2
```
| Flag | Coverage Ξ | |
|---|---|---|
| javascript | `53.33% <17.18%> (-0.06%)` | :arrow_down: |
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/21974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Ξ | |
|---|---|---|
| [...rontend/src/components/DropdownContainer/index.tsx](https://codecov.io/gh/apache/superset/pull/21974/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRHJvcGRvd25Db250YWluZXIvaW5kZXgudHN4) | `0.00% <0.00%> (ΓΈ)` | |
| [superset-frontend/src/components/Select/styles.tsx](https://codecov.io/gh/apache/superset/pull/21974/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L3N0eWxlcy50c3g=) | `81.81% <63.63%> (-18.19%)` | :arrow_down: |
| [...set-frontend/src/components/Select/AsyncSelect.tsx](https://codecov.io/gh/apache/superset/pull/21974/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==) | `87.84% <100.00%> (+0.13%)` | :arrow_up: |
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/21974/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=) | `92.70% <100.00%> (+0.15%)` | :arrow_up: |
| [...ashboard/components/BuilderComponentPane/index.tsx](https://codecov.io/gh/apache/superset/pull/21974/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0J1aWxkZXJDb21wb25lbnRQYW5lL2luZGV4LnRzeA==) | `66.66% <0.00%> (-9.81%)` | :arrow_down: |
| [...ters/FiltersConfigModal/FiltersConfigForm/utils.ts](https://codecov.io/gh/apache/superset/pull/21974/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL3V0aWxzLnRz) | `77.77% <0.00%> (-4.58%)` | :arrow_down: |
| [...t-frontend/src/dashboard/components/SliceAdder.jsx](https://codecov.io/gh/apache/superset/pull/21974/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1NsaWNlQWRkZXIuanN4) | `60.27% <0.00%> (-2.40%)` | :arrow_down: |
| [...frontend/src/dashboard/components/Header/index.jsx](https://codecov.io/gh/apache/superset/pull/21974/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0hlYWRlci9pbmRleC5qc3g=) | `58.99% <0.00%> (-0.87%)` | :arrow_down: |
| [...et-frontend/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/superset/pull/21974/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZXhwbG9yZVJlZHVjZXIuanM=) | `38.09% <0.00%> (-0.62%)` | :arrow_down: |
| [...d/src/SqlLab/components/SaveDatasetModal/index.tsx](https://codecov.io/gh/apache/superset/pull/21974/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVEYXRhc2V0TW9kYWwvaW5kZXgudHN4) | `52.27% <0.00%> (-0.61%)` | :arrow_down: |
| ... and [36 more](https://codecov.io/gh/apache/superset/pull/21974/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
: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] kgabryje merged pull request #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
kgabryje merged PR #21974:
URL: https://github.com/apache/superset/pull/21974
--
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 #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #21974:
URL: https://github.com/apache/superset/pull/21974#issuecomment-1296948699
Awesome job! 1 suggestion - in the recording we can see some flashing of appearing/disappearing elements during resizing. I'm assuming that happens when container size is on the verge of fitting or not an additional element. Could we maybe add a few pixels hysteresis to changing the state of displayed/hidden elements?
--
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 a diff in pull request #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #21974:
URL: https://github.com/apache/superset/pull/21974#discussion_r1009301212
##########
superset-frontend/src/components/DropdownContainer/index.tsx:
##########
@@ -0,0 +1,233 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { CSSProperties, useEffect, useMemo, useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { useResizeDetector } from 'react-resize-detector';
+import { usePrevious } from 'src/hooks/usePrevious';
+import Badge from '../Badge';
+import Icons from '../Icons';
+import Button from '../Button';
+import Popover from '../Popover';
+
+/**
+ * Horizontal container that displays overflowed items in a popover.
+ * It shows an indicator of how many items are currently overflowed.
+ */
+export interface DropdownContainerProps {
+ /**
+ * Array of items. The id property is used to uniquely identify
+ * the elements when rendering or dealing with event handlers.
+ */
+ items: {
+ id: string;
+ element: React.ReactElement;
+ }[];
+ /**
+ * Event handler called every time an element moves between
+ * main container and popover.
+ */
+ onOverflowingStateChange?: (overflowingState: {
+ notOverflowed: string[];
+ overflowed: string[];
+ }) => void;
+ /**
+ * Popover additional style properties.
+ */
+ popoverStyle?: CSSProperties;
+ /**
+ * Icon of the popover trigger.
+ */
+ popoverTriggerIcon?: React.ReactElement;
+ /**
+ * Text of the popover trigger.
+ */
+ popoverTriggerText?: string;
+ /**
+ * Main container additional style properties.
+ */
+ style?: CSSProperties;
+}
+
+const DropdownContainer = ({
+ items,
+ onOverflowingStateChange,
+ popoverStyle = {},
+ popoverTriggerIcon,
+ popoverTriggerText = t('More'),
+ style,
+}: DropdownContainerProps) => {
+ const theme = useTheme();
+ const { ref, width = 0 } = useResizeDetector<HTMLDivElement>();
+ const previousWidth = usePrevious(width) || 0;
+ const { current } = ref;
+ const [overflowingIndex, setOverflowingIndex] = useState<number>(-1);
+ const [itemsWidth, setItemsWidth] = useState<number[]>([]);
+
+ useEffect(() => {
+ const container = current?.children.item(0);
+ if (container) {
+ const { children } = container;
+ const childrenArray = Array.from(children);
+
+ // Stores items width once
+ if (itemsWidth.length === 0) {
+ setItemsWidth(
+ childrenArray.map(child => child.getBoundingClientRect().width),
+ );
+ }
+
+ // Calculates the index of the first overflowed element
+ const index = childrenArray.findIndex(
+ child =>
+ child.getBoundingClientRect().right >
+ container.getBoundingClientRect().right,
+ );
+ setOverflowingIndex(index === -1 ? children.length : index);
+
+ if (width > previousWidth && overflowingIndex !== -1) {
+ // Calculates remaining space in the container
+ const button = current?.children.item(1);
+ const buttonRight = button?.getBoundingClientRect().right || 0;
+ const containerRight = current?.getBoundingClientRect().right || 0;
+ const remainingSpace = containerRight - buttonRight;
+ // Checks if the first element in the popover fits in the remaining space
+ const fitsInRemainingSpace = remainingSpace >= itemsWidth[0];
+ if (fitsInRemainingSpace && overflowingIndex < items.length) {
+ // Moves element from popover to container
+ setOverflowingIndex(overflowingIndex + 1);
+ }
+ }
+ }
+ }, [
+ current,
+ items.length,
+ itemsWidth,
+ overflowingIndex,
+ previousWidth,
+ width,
+ ]);
+
+ const notOverflowedItems = useMemo(
+ () =>
+ items
+ .slice(0, overflowingIndex !== -1 ? overflowingIndex : items.length)
+ .map(item => React.cloneElement(item.element, { key: item.id })),
+ [items, overflowingIndex],
+ );
+
+ const notOverflowedIds = useMemo(
+ () =>
+ items
+ .slice(0, overflowingIndex !== -1 ? overflowingIndex : items.length)
+ .map(item => item.id),
+ [items, overflowingIndex],
+ );
+
+ const overflowedItems = useMemo(
+ () =>
+ overflowingIndex !== -1
+ ? items
+ .slice(overflowingIndex, items.length)
+ .map(item => React.cloneElement(item.element, { key: item.id }))
+ : [],
+ [items, overflowingIndex],
+ );
+
+ const overflowedIds = useMemo(
+ () =>
+ overflowingIndex !== -1
+ ? items.slice(overflowingIndex, items.length).map(item => item.id)
+ : [],
+ [items, overflowingIndex],
+ );
+
+ useEffect(() => {
+ if (onOverflowingStateChange) {
+ onOverflowingStateChange({
+ notOverflowed: notOverflowedIds,
+ overflowed: overflowedIds,
+ });
+ }
+ }, [notOverflowedIds, onOverflowingStateChange, overflowedIds]);
+
+ const popoverContent = useMemo(
+ () => (
+ <div
+ css={css`
+ display: flex;
+ flex-direction: column;
+ gap: ${theme.gridUnit * 3}px;
+ width: 200px;
+ `}
+ style={popoverStyle}
+ >
+ {overflowedItems}
Review Comment:
Can we make the popover content customizable via props as well? In the case of horizontal filter bar we'll need to hide filters out of scope in a collapsible inside of popover
--
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] codyml commented on pull request #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #21974:
URL: https://github.com/apache/superset/pull/21974#issuecomment-1299118233
@michael-s-molina For the horizontal filter bar, I think we may also need a way to programmatically open/close the dropdown. Would that be possible to include here, maybe with a `useImperativeHandle`? I think we'll also want to be able to scroll to and highlight overflow content, so could we also maybe expose a ref to the dropdown content container?
--
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 a diff in pull request #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #21974:
URL: https://github.com/apache/superset/pull/21974#discussion_r1009311077
##########
superset-frontend/src/components/DropdownContainer/index.tsx:
##########
@@ -0,0 +1,233 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { CSSProperties, useEffect, useMemo, useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { useResizeDetector } from 'react-resize-detector';
+import { usePrevious } from 'src/hooks/usePrevious';
+import Badge from '../Badge';
+import Icons from '../Icons';
+import Button from '../Button';
+import Popover from '../Popover';
+
+/**
+ * Horizontal container that displays overflowed items in a popover.
+ * It shows an indicator of how many items are currently overflowed.
+ */
+export interface DropdownContainerProps {
+ /**
+ * Array of items. The id property is used to uniquely identify
+ * the elements when rendering or dealing with event handlers.
+ */
+ items: {
+ id: string;
+ element: React.ReactElement;
+ }[];
+ /**
+ * Event handler called every time an element moves between
+ * main container and popover.
+ */
+ onOverflowingStateChange?: (overflowingState: {
+ notOverflowed: string[];
+ overflowed: string[];
+ }) => void;
+ /**
+ * Popover additional style properties.
+ */
+ popoverStyle?: CSSProperties;
+ /**
+ * Icon of the popover trigger.
+ */
+ popoverTriggerIcon?: React.ReactElement;
+ /**
+ * Text of the popover trigger.
+ */
+ popoverTriggerText?: string;
+ /**
+ * Main container additional style properties.
+ */
+ style?: CSSProperties;
+}
+
+const DropdownContainer = ({
+ items,
+ onOverflowingStateChange,
+ popoverStyle = {},
+ popoverTriggerIcon,
+ popoverTriggerText = t('More'),
+ style,
+}: DropdownContainerProps) => {
+ const theme = useTheme();
+ const { ref, width = 0 } = useResizeDetector<HTMLDivElement>();
+ const previousWidth = usePrevious(width) || 0;
+ const { current } = ref;
+ const [overflowingIndex, setOverflowingIndex] = useState<number>(-1);
+ const [itemsWidth, setItemsWidth] = useState<number[]>([]);
+
+ useEffect(() => {
Review Comment:
I think `useLayoutEffect` is preferred when doing stuff like layout measurements https://dev.to/emmanuelthecoder/useeffect-vs-uselayouteffect-the-difference-and-when-to-use-them-124c
--
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 #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #21974:
URL: https://github.com/apache/superset/pull/21974#issuecomment-1296000895
I already have RTL tests locally but some are still failing. I'll add them soon. I'll also add an MDX file with an overview of the 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] michael-s-molina commented on a diff in pull request #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21974:
URL: https://github.com/apache/superset/pull/21974#discussion_r1009651558
##########
superset-frontend/src/components/DropdownContainer/index.tsx:
##########
@@ -0,0 +1,233 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { CSSProperties, useEffect, useMemo, useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { useResizeDetector } from 'react-resize-detector';
+import { usePrevious } from 'src/hooks/usePrevious';
+import Badge from '../Badge';
+import Icons from '../Icons';
+import Button from '../Button';
+import Popover from '../Popover';
+
+/**
+ * Horizontal container that displays overflowed items in a popover.
+ * It shows an indicator of how many items are currently overflowed.
+ */
+export interface DropdownContainerProps {
+ /**
+ * Array of items. The id property is used to uniquely identify
+ * the elements when rendering or dealing with event handlers.
+ */
+ items: {
+ id: string;
+ element: React.ReactElement;
+ }[];
+ /**
+ * Event handler called every time an element moves between
+ * main container and popover.
+ */
+ onOverflowingStateChange?: (overflowingState: {
+ notOverflowed: string[];
+ overflowed: string[];
+ }) => void;
+ /**
+ * Popover additional style properties.
+ */
+ popoverStyle?: CSSProperties;
+ /**
+ * Icon of the popover trigger.
+ */
+ popoverTriggerIcon?: React.ReactElement;
+ /**
+ * Text of the popover trigger.
+ */
+ popoverTriggerText?: string;
+ /**
+ * Main container additional style properties.
+ */
+ style?: CSSProperties;
+}
+
+const DropdownContainer = ({
+ items,
+ onOverflowingStateChange,
+ popoverStyle = {},
+ popoverTriggerIcon,
+ popoverTriggerText = t('More'),
+ style,
+}: DropdownContainerProps) => {
+ const theme = useTheme();
+ const { ref, width = 0 } = useResizeDetector<HTMLDivElement>();
+ const previousWidth = usePrevious(width) || 0;
+ const { current } = ref;
+ const [overflowingIndex, setOverflowingIndex] = useState<number>(-1);
+ const [itemsWidth, setItemsWidth] = useState<number[]>([]);
+
+ useEffect(() => {
+ const container = current?.children.item(0);
+ if (container) {
+ const { children } = container;
+ const childrenArray = Array.from(children);
+
+ // Stores items width once
+ if (itemsWidth.length === 0) {
+ setItemsWidth(
+ childrenArray.map(child => child.getBoundingClientRect().width),
+ );
+ }
+
+ // Calculates the index of the first overflowed element
+ const index = childrenArray.findIndex(
+ child =>
+ child.getBoundingClientRect().right >
+ container.getBoundingClientRect().right,
+ );
+ setOverflowingIndex(index === -1 ? children.length : index);
+
+ if (width > previousWidth && overflowingIndex !== -1) {
+ // Calculates remaining space in the container
+ const button = current?.children.item(1);
+ const buttonRight = button?.getBoundingClientRect().right || 0;
+ const containerRight = current?.getBoundingClientRect().right || 0;
+ const remainingSpace = containerRight - buttonRight;
+ // Checks if the first element in the popover fits in the remaining space
+ const fitsInRemainingSpace = remainingSpace >= itemsWidth[0];
+ if (fitsInRemainingSpace && overflowingIndex < items.length) {
+ // Moves element from popover to container
+ setOverflowingIndex(overflowingIndex + 1);
+ }
+ }
+ }
+ }, [
+ current,
+ items.length,
+ itemsWidth,
+ overflowingIndex,
+ previousWidth,
+ width,
+ ]);
+
+ const notOverflowedItems = useMemo(
+ () =>
+ items
+ .slice(0, overflowingIndex !== -1 ? overflowingIndex : items.length)
+ .map(item => React.cloneElement(item.element, { key: item.id })),
+ [items, overflowingIndex],
+ );
+
+ const notOverflowedIds = useMemo(
+ () =>
+ items
+ .slice(0, overflowingIndex !== -1 ? overflowingIndex : items.length)
+ .map(item => item.id),
+ [items, overflowingIndex],
+ );
+
+ const overflowedItems = useMemo(
+ () =>
+ overflowingIndex !== -1
+ ? items
+ .slice(overflowingIndex, items.length)
+ .map(item => React.cloneElement(item.element, { key: item.id }))
+ : [],
+ [items, overflowingIndex],
+ );
+
+ const overflowedIds = useMemo(
Review Comment:
Done ππΌ
--
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 a diff in pull request #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #21974:
URL: https://github.com/apache/superset/pull/21974#discussion_r1009302333
##########
superset-frontend/src/components/DropdownContainer/index.tsx:
##########
@@ -0,0 +1,233 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { CSSProperties, useEffect, useMemo, useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { useResizeDetector } from 'react-resize-detector';
+import { usePrevious } from 'src/hooks/usePrevious';
+import Badge from '../Badge';
+import Icons from '../Icons';
+import Button from '../Button';
+import Popover from '../Popover';
+
+/**
+ * Horizontal container that displays overflowed items in a popover.
+ * It shows an indicator of how many items are currently overflowed.
+ */
+export interface DropdownContainerProps {
+ /**
+ * Array of items. The id property is used to uniquely identify
+ * the elements when rendering or dealing with event handlers.
+ */
+ items: {
+ id: string;
+ element: React.ReactElement;
+ }[];
+ /**
+ * Event handler called every time an element moves between
+ * main container and popover.
+ */
+ onOverflowingStateChange?: (overflowingState: {
+ notOverflowed: string[];
+ overflowed: string[];
+ }) => void;
+ /**
+ * Popover additional style properties.
+ */
+ popoverStyle?: CSSProperties;
+ /**
+ * Icon of the popover trigger.
+ */
+ popoverTriggerIcon?: React.ReactElement;
+ /**
+ * Text of the popover trigger.
+ */
+ popoverTriggerText?: string;
+ /**
+ * Main container additional style properties.
+ */
+ style?: CSSProperties;
+}
+
+const DropdownContainer = ({
+ items,
+ onOverflowingStateChange,
+ popoverStyle = {},
+ popoverTriggerIcon,
+ popoverTriggerText = t('More'),
+ style,
+}: DropdownContainerProps) => {
+ const theme = useTheme();
+ const { ref, width = 0 } = useResizeDetector<HTMLDivElement>();
+ const previousWidth = usePrevious(width) || 0;
+ const { current } = ref;
+ const [overflowingIndex, setOverflowingIndex] = useState<number>(-1);
+ const [itemsWidth, setItemsWidth] = useState<number[]>([]);
+
+ useEffect(() => {
+ const container = current?.children.item(0);
+ if (container) {
+ const { children } = container;
+ const childrenArray = Array.from(children);
+
+ // Stores items width once
+ if (itemsWidth.length === 0) {
+ setItemsWidth(
+ childrenArray.map(child => child.getBoundingClientRect().width),
+ );
+ }
+
+ // Calculates the index of the first overflowed element
+ const index = childrenArray.findIndex(
+ child =>
+ child.getBoundingClientRect().right >
+ container.getBoundingClientRect().right,
+ );
+ setOverflowingIndex(index === -1 ? children.length : index);
+
+ if (width > previousWidth && overflowingIndex !== -1) {
+ // Calculates remaining space in the container
+ const button = current?.children.item(1);
+ const buttonRight = button?.getBoundingClientRect().right || 0;
+ const containerRight = current?.getBoundingClientRect().right || 0;
+ const remainingSpace = containerRight - buttonRight;
+ // Checks if the first element in the popover fits in the remaining space
+ const fitsInRemainingSpace = remainingSpace >= itemsWidth[0];
+ if (fitsInRemainingSpace && overflowingIndex < items.length) {
+ // Moves element from popover to container
+ setOverflowingIndex(overflowingIndex + 1);
+ }
+ }
+ }
+ }, [
+ current,
+ items.length,
+ itemsWidth,
+ overflowingIndex,
+ previousWidth,
+ width,
+ ]);
+
+ const notOverflowedItems = useMemo(
+ () =>
+ items
+ .slice(0, overflowingIndex !== -1 ? overflowingIndex : items.length)
+ .map(item => React.cloneElement(item.element, { key: item.id })),
+ [items, overflowingIndex],
+ );
+
+ const notOverflowedIds = useMemo(
+ () =>
+ items
+ .slice(0, overflowingIndex !== -1 ? overflowingIndex : items.length)
+ .map(item => item.id),
+ [items, overflowingIndex],
+ );
+
+ const overflowedItems = useMemo(
+ () =>
+ overflowingIndex !== -1
+ ? items
+ .slice(overflowingIndex, items.length)
+ .map(item => React.cloneElement(item.element, { key: item.id }))
+ : [],
+ [items, overflowingIndex],
+ );
+
+ const overflowedIds = useMemo(
Review Comment:
Could we maybe merge calculating items and ids in 1 operation? Like `const [overflowedItems, overflowedIds] = useMemo(...)`
--
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 #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21974:
URL: https://github.com/apache/superset/pull/21974#discussion_r1009651930
##########
superset-frontend/src/components/DropdownContainer/index.tsx:
##########
@@ -0,0 +1,233 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { CSSProperties, useEffect, useMemo, useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { useResizeDetector } from 'react-resize-detector';
+import { usePrevious } from 'src/hooks/usePrevious';
+import Badge from '../Badge';
+import Icons from '../Icons';
+import Button from '../Button';
+import Popover from '../Popover';
+
+/**
+ * Horizontal container that displays overflowed items in a popover.
+ * It shows an indicator of how many items are currently overflowed.
+ */
+export interface DropdownContainerProps {
+ /**
+ * Array of items. The id property is used to uniquely identify
+ * the elements when rendering or dealing with event handlers.
+ */
+ items: {
+ id: string;
+ element: React.ReactElement;
+ }[];
+ /**
+ * Event handler called every time an element moves between
+ * main container and popover.
+ */
+ onOverflowingStateChange?: (overflowingState: {
+ notOverflowed: string[];
+ overflowed: string[];
+ }) => void;
+ /**
+ * Popover additional style properties.
+ */
+ popoverStyle?: CSSProperties;
+ /**
+ * Icon of the popover trigger.
+ */
+ popoverTriggerIcon?: React.ReactElement;
+ /**
+ * Text of the popover trigger.
+ */
+ popoverTriggerText?: string;
+ /**
+ * Main container additional style properties.
+ */
+ style?: CSSProperties;
+}
+
+const DropdownContainer = ({
+ items,
+ onOverflowingStateChange,
+ popoverStyle = {},
+ popoverTriggerIcon,
+ popoverTriggerText = t('More'),
+ style,
+}: DropdownContainerProps) => {
+ const theme = useTheme();
+ const { ref, width = 0 } = useResizeDetector<HTMLDivElement>();
+ const previousWidth = usePrevious(width) || 0;
+ const { current } = ref;
+ const [overflowingIndex, setOverflowingIndex] = useState<number>(-1);
+ const [itemsWidth, setItemsWidth] = useState<number[]>([]);
+
+ useEffect(() => {
Review Comment:
Done ππΌ
--
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] codyml commented on pull request #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #21974:
URL: https://github.com/apache/superset/pull/21974#issuecomment-1299106093
@michael-s-molina This looks great, thanks! Would it be possible to add a prop for customizing the value in the badge? For horizontal filter bar, I believe we'll need to show the number of active filters, not the total number of filters. If there was a badge count prop, we could handle calls to `onOverflowingStateChange` by checking which overflowing filters in the dropdown are active and setting the badge accordingly.
--
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 #21974: feat: Adds the DropdownContainer component
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #21974:
URL: https://github.com/apache/superset/pull/21974#issuecomment-1302150324
Merged to unblock other tasks, we'll address @codyml's comments in other PRs
--
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