michael-s-molina commented on code in PR #22502:
URL: https://github.com/apache/superset/pull/22502#discussion_r1055673387
##########
superset-frontend/src/hooks/useTruncation/useChildElementTruncation.ts:
##########
@@ -27,92 +27,65 @@ import { RefObject, useLayoutEffect, useState, useRef }
from 'react';
* (including those completely hidden) and whether any elements
* are completely hidden.
Review Comment:
Can you delete the original description? We don't receive refs anymore.
##########
superset-frontend/src/hooks/useTruncation/useChildElementTruncation.ts:
##########
@@ -27,92 +27,65 @@ import { RefObject, useLayoutEffect, useState, useRef }
from 'react';
* (including those completely hidden) and whether any elements
* are completely hidden.
*/
-const useChildElementTruncation = (
- elementRef: RefObject<HTMLElement>,
- plusRef?: RefObject<HTMLElement>,
-) => {
- const [elementsTruncated, setElementsTruncated] = useState(0);
- const [hasHiddenElements, setHasHiddenElements] = useState(false);
+// Using DEFAULT_ARRAY the dependency array is not getting a new array every
time this runs for
+// an null / undefined elementRef or elementRef with no childeNodes
+const DEFAULT_ARRAY: HTMLElement[] = [];
- const previousEffectInfoRef = useRef({
+/**
+ * Evaluates if an HTMLElement has truncated (clipped / partialy visible /
hidden) content
+ * and calculates the number of child elements that are completely hidden.
+ * NOTE: Any child element that has at least 1px of visible content
+ * will be excluded from the hiddenElementCount.
+ * @param elementRef HTMLElement that will be checked to see if the sum of
child element widths
+ * exceeds the visible clientWidth of the elementRef.
+ * @returns [isTruncated: boolean, hiddenElementCount: number]
+ */
+const useChildElementTruncation = (elementRef: RefObject<HTMLElement>) => {
+ const [isTruncated, setIsTruncated] = useState<boolean>(false);
+ const [hiddenElementCount, setHiddenElementCount] = useState<number>(0);
+
+ const { scrollWidth, clientWidth, childNodes } = elementRef?.current ?? {
scrollWidth: 0,
- parentElementWidth: 0,
- plusRefWidth: 0,
- });
+ clientWidth: 0,
+ childNodes: DEFAULT_ARRAY,
+ };
useLayoutEffect(() => {
- const currentElement = elementRef.current;
- const plusRefElement = plusRef?.current;
-
- if (!currentElement) {
- return;
- }
-
- const { scrollWidth, clientWidth, childNodes } = currentElement;
-
- // By using the result of this effect to truncate content
- // we're effectively changing it's size.
- // That will trigger another pass at this effect.
- // Depending on the content elements width, that second rerender could
- // yield a different truncate count, thus potentially leading to a
- // rendering loop.
- // There's only a need to recompute if the parent width or the width of
- // the child nodes changes.
- const previousEffectInfo = previousEffectInfoRef.current;
- const parentElementWidth = currentElement.parentElement?.clientWidth || 0;
- const plusRefWidth = plusRefElement?.offsetWidth || 0;
- previousEffectInfoRef.current = {
- scrollWidth,
- parentElementWidth,
- plusRefWidth,
- };
-
- if (
- previousEffectInfo.parentElementWidth === parentElementWidth &&
- previousEffectInfo.scrollWidth === scrollWidth &&
- previousEffectInfo.plusRefWidth === plusRefWidth
- ) {
- return;
- }
-
if (scrollWidth > clientWidth) {
- // "..." is around 6px wide
- const truncationWidth = 6;
- const plusSize = plusRefElement?.offsetWidth || 0;
- const maxWidth = clientWidth - truncationWidth;
const elementsCount = childNodes.length;
let width = 0;
- let hiddenElements = 0;
- for (let i = 0; i < elementsCount; i += 1) {
- const itemWidth = (childNodes[i] as HTMLElement).offsetWidth;
- const remainingWidth = maxWidth - truncationWidth - width - plusSize;
-
- // assures it shows +{number} only when the item is not visible
+ let hiddenElementCounter = 0;
+ childNodes.forEach((node: HTMLElement) => {
+ const itemWidth = node?.offsetWidth;
Review Comment:
If we want to support
[transform](https://developer.mozilla.org/en-US/docs/Web/CSS/transform), then
[Element.getBoundingClientRect()](https://developer.mozilla.org/en-US/docs/Web/API/CSS_Object_Model/Determining_the_dimensions_of_elements#how_much_room_does_it_use_up)
would be more suitable.
##########
superset-frontend/src/hooks/useTruncation/useChildElementTruncation.ts:
##########
@@ -27,92 +27,65 @@ import { RefObject, useLayoutEffect, useState, useRef }
from 'react';
* (including those completely hidden) and whether any elements
* are completely hidden.
*/
-const useChildElementTruncation = (
- elementRef: RefObject<HTMLElement>,
- plusRef?: RefObject<HTMLElement>,
-) => {
- const [elementsTruncated, setElementsTruncated] = useState(0);
- const [hasHiddenElements, setHasHiddenElements] = useState(false);
+// Using DEFAULT_ARRAY the dependency array is not getting a new array every
time this runs for
+// an null / undefined elementRef or elementRef with no childeNodes
+const DEFAULT_ARRAY: HTMLElement[] = [];
- const previousEffectInfoRef = useRef({
+/**
+ * Evaluates if an HTMLElement has truncated (clipped / partialy visible /
hidden) content
+ * and calculates the number of child elements that are completely hidden.
+ * NOTE: Any child element that has at least 1px of visible content
+ * will be excluded from the hiddenElementCount.
+ * @param elementRef HTMLElement that will be checked to see if the sum of
child element widths
+ * exceeds the visible clientWidth of the elementRef.
+ * @returns [isTruncated: boolean, hiddenElementCount: number]
+ */
+const useChildElementTruncation = (elementRef: RefObject<HTMLElement>) => {
+ const [isTruncated, setIsTruncated] = useState<boolean>(false);
+ const [hiddenElementCount, setHiddenElementCount] = useState<number>(0);
+
+ const { scrollWidth, clientWidth, childNodes } = elementRef?.current ?? {
scrollWidth: 0,
- parentElementWidth: 0,
- plusRefWidth: 0,
- });
+ clientWidth: 0,
+ childNodes: DEFAULT_ARRAY,
+ };
useLayoutEffect(() => {
- const currentElement = elementRef.current;
- const plusRefElement = plusRef?.current;
-
- if (!currentElement) {
- return;
- }
-
- const { scrollWidth, clientWidth, childNodes } = currentElement;
-
- // By using the result of this effect to truncate content
- // we're effectively changing it's size.
- // That will trigger another pass at this effect.
- // Depending on the content elements width, that second rerender could
- // yield a different truncate count, thus potentially leading to a
- // rendering loop.
- // There's only a need to recompute if the parent width or the width of
- // the child nodes changes.
- const previousEffectInfo = previousEffectInfoRef.current;
- const parentElementWidth = currentElement.parentElement?.clientWidth || 0;
- const plusRefWidth = plusRefElement?.offsetWidth || 0;
- previousEffectInfoRef.current = {
- scrollWidth,
- parentElementWidth,
- plusRefWidth,
- };
-
- if (
- previousEffectInfo.parentElementWidth === parentElementWidth &&
- previousEffectInfo.scrollWidth === scrollWidth &&
- previousEffectInfo.plusRefWidth === plusRefWidth
- ) {
- return;
- }
-
if (scrollWidth > clientWidth) {
- // "..." is around 6px wide
- const truncationWidth = 6;
- const plusSize = plusRefElement?.offsetWidth || 0;
- const maxWidth = clientWidth - truncationWidth;
const elementsCount = childNodes.length;
let width = 0;
- let hiddenElements = 0;
- for (let i = 0; i < elementsCount; i += 1) {
- const itemWidth = (childNodes[i] as HTMLElement).offsetWidth;
- const remainingWidth = maxWidth - truncationWidth - width - plusSize;
-
- // assures it shows +{number} only when the item is not visible
+ let hiddenElementCounter = 0;
+ childNodes.forEach((node: HTMLElement) => {
+ const itemWidth = node?.offsetWidth;
+ const remainingWidth = clientWidth - width;
+ /**
+ * Evaluate if hiddenElementCounter should increment before adding to
width
+ * to ensure we do not count partially hidden / clipped / truncated
element as a hidden item
+ */
Review Comment:
Just to make it similar to the other comments.
```suggestion
// Evaluate if hiddenElementCounter should increment before adding
to width
// to ensure we do not count partially hidden / clipped / truncated
element as a hidden item
```
--
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-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
