From e95df2672d7a67eb30df839e0d81f2d79d1ccb2e Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Fri, 13 Nov 2020 18:12:16 +0000 Subject: [PATCH 01/20] [KED-1923] Refactor and memoise NodeListRow --- src/components/node-list/node-list-row.js | 229 ++++++++++++---------- src/utils/changed.js | 9 + 2 files changed, 135 insertions(+), 103 deletions(-) create mode 100644 src/utils/changed.js diff --git a/src/components/node-list/node-list-row.js b/src/components/node-list/node-list-row.js index 149f91a06f..ae7ab32e13 100644 --- a/src/components/node-list/node-list-row.js +++ b/src/components/node-list/node-list-row.js @@ -1,122 +1,145 @@ -import React from 'react'; +import React, { memo } from 'react'; import { connect } from 'react-redux'; import classnames from 'classnames'; +import changed from '../../utils/changed'; import NodeIcon from '../icons/node-icon'; import VisibleIcon from '../icons/visible'; import InvisibleIcon from '../icons/invisible'; import { getNodeActive } from '../../selectors/nodes'; -const NodeListRow = ({ - container: Container = 'div', - active, - checked, - unset, - children, - disabled, - faded, - visible, - id, - label, - name, - kind, - onMouseEnter, - onMouseLeave, - onChange, - onClick, - selected, - type, - visibleIcon = VisibleIcon, - invisibleIcon = InvisibleIcon -}) => { - const VisibilityIcon = checked ? visibleIcon : invisibleIcon; +export const nodeListRowHeight = 36.59375; - return ( - - + {children} + - ); -}; + + + ); + }, + shouldMemo +); export const mapStateToProps = (state, ownProps) => ({ ...ownProps, diff --git a/src/utils/changed.js b/src/utils/changed.js new file mode 100644 index 0000000000..5861f92fb4 --- /dev/null +++ b/src/utils/changed.js @@ -0,0 +1,9 @@ +/** + * Returns true if any of the given props are different between given objects. + * Only shallow changes are detected. + */ +export default (props, objectA, objectB) => { + return ( + objectA && objectB && props.some(prop => objectA[prop] !== objectB[prop]) + ); +}; From 44f9a0f0eeb8c68852ae927f129dfeb105991e7d Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Fri, 13 Nov 2020 18:14:05 +0000 Subject: [PATCH 02/20] [KED-1923] Add LazyList component --- src/components/lazy-list/index.js | 180 ++++++++++++++++++++++++++++++ 1 file changed, 180 insertions(+) create mode 100644 src/components/lazy-list/index.js diff --git a/src/components/lazy-list/index.js b/src/components/lazy-list/index.js new file mode 100644 index 0000000000..64c9e9baf1 --- /dev/null +++ b/src/components/lazy-list/index.js @@ -0,0 +1,180 @@ +import { useState, useLayoutEffect, useRef, useMemo, useCallback } from 'react'; + +const supported = typeof IntersectionObserver !== 'undefined'; + +export default ({ + children, + height, + total, + onChange, + lazy = true, + dispose = false, + buffer = 0.5 +}) => { + const active = lazy && supported; + + const [range, setRange] = useState([0, 0]); + const nextRange = useRef([0, 0]); + + const listRef = useRef(); + const lowerRef = useRef(); + const upperRef = useRef(); + + const itemHeight = useMemo(() => height(0, 1), [height]); + const totalHeight = useMemo(() => height(0, total), [height, total]); + const upperHeight = useMemo(() => height(0, range[0]), [height, range]); + const lowerHeight = useMemo(() => height(range[1], total), [ + height, + range, + total + ]); + + if (active) { + const requestUpdate = useRequestFrameOnce( + useCallback(() => { + const visibleRange = visibleRangeOf( + listRef.current, + listRef.current?.offsetParent, + buffer, + total, + itemHeight + ); + + const effectiveRange = dispose + ? visibleRange + : rangeUnion(nextRange.current, visibleRange); + + // Avoid duplicate renders if ranges are equal + if (!rangeEqual(nextRange.current, effectiveRange)) { + nextRange.current = effectiveRange; + setRange(effectiveRange); + } + }, [buffer, total, itemHeight, dispose]) + ); + + const observerOptions = useMemo(() => ({ threshold: thresholds(total) }), [ + total + ]); + + useIntersection(listRef, observerOptions, requestUpdate); + useIntersection(upperRef, observerOptions, requestUpdate); + useIntersection(lowerRef, observerOptions, requestUpdate); + + useLayoutEffect(() => requestUpdate(), [total, totalHeight, requestUpdate]); + } + + const childProps = useMemo( + () => ({ + start: active ? range[0] : 0, + end: active ? range[1] : total, + total, + listRef, + upperRef, + lowerRef, + listStyle: { + position: 'relative', + height: active ? totalHeight : undefined, + paddingTop: active ? upperHeight : undefined + }, + upperStyle: { + position: 'absolute', + display: !active ? 'none' : undefined, + height: upperHeight, + width: '100%', + top: '0' + }, + lowerStyle: { + position: 'absolute', + display: !active ? 'none' : undefined, + height: lowerHeight, + width: '100%', + bottom: '0' + } + }), + [ + active, + range, + total, + listRef, + upperRef, + lowerRef, + totalHeight, + upperHeight, + lowerHeight + ] + ); + + onChange && onChange(childProps); + + return children(childProps); +}; + +const range = (start, end, min, max) => [ + Math.max(Math.min(start, max), min), + Math.max(Math.min(end, max), min) +]; + +const rangeUnion = (rangeA, rangeB) => [ + Math.min(rangeA[0], rangeB[0]), + Math.max(rangeA[1], rangeB[1]) +]; + +const rangeEqual = (rangeA, rangeB) => + rangeA[0] === rangeB[0] && rangeA[1] === rangeB[1]; + +const visibleRangeOf = (element, container, buffer, total, itemHeight) => { + if (!element || !container) { + return [0, 0]; + } + + const rect = element.getBoundingClientRect(); + const clip = container.getBoundingClientRect(); + + const bufferCount = Math.ceil((buffer * clip.height) / itemHeight); + + if (rect.bottom < clip.top) { + return range(total - bufferCount, total, 0, total); + } + + if (rect.top > clip.bottom) { + return range(0, bufferCount, 0, total); + } + + const top = Math.min(Math.max(rect.top, clip.top), clip.bottom); + const bottom = Math.max(Math.min(rect.bottom, clip.bottom), clip.top); + + const start = Math.floor((top - rect.top) / itemHeight); + const end = Math.ceil((bottom - rect.top) / itemHeight); + + return range(start - bufferCount, end + bufferCount, 0, total); +}; + +const useRequestFrameOnce = callback => { + const request = useRef(); + + // Keep the callback cached to avoid constant re-creation + return useCallback(() => { + cancelAnimationFrame(request.current); + request.current = requestAnimationFrame(callback); + }, [request, callback]); +}; + +const thresholds = total => + Array.from({ length: total }, (_, i) => i / (total - 1)); + +const useIntersection = (element, options, callback) => { + const observer = useRef(); + + // Must be an effect to avoid constant re-creation + return useLayoutEffect(() => { + if (!element.current) { + return; + } + if (observer.current) { + observer.current.disconnect(); + } + observer.current = new IntersectionObserver(callback, options); + observer.current.observe(element.current); + callback(); + }, [callback, element, options]); +}; From d1be957171c5738eee7a16bc4c0a9a71ec938bd1 Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Fri, 13 Nov 2020 18:19:37 +0000 Subject: [PATCH 03/20] [KED-1923] Refactor NodeListGroup, NodeListGroups using LazyList --- src/components/node-list/node-list-group.js | 97 +++++++++++++++++--- src/components/node-list/node-list-groups.js | 38 ++------ 2 files changed, 90 insertions(+), 45 deletions(-) diff --git a/src/components/node-list/node-list-group.js b/src/components/node-list/node-list-group.js index 65f4355226..ae84e527b3 100644 --- a/src/components/node-list/node-list-group.js +++ b/src/components/node-list/node-list-group.js @@ -1,12 +1,12 @@ import React from 'react'; import classnames from 'classnames'; -import NodeListRow from './node-list-row'; +import modifiers from '../../utils/modifiers'; +import NodeListRow, { nodeListRowHeight } from './node-list-row'; +import LazyList from '../lazy-list'; export const NodeListGroup = ({ - container: Container = 'div', - childrenContainer: ChildrenContainer = 'div', - childrenClassName, - children, + items, + group, collapsed, id, name, @@ -18,9 +18,13 @@ export const NodeListGroup = ({ visibleIcon, invisibleIcon, onToggleChecked, - onToggleCollapsed + onToggleCollapsed, + onItemClick, + onItemChange, + onItemMouseEnter, + onItemMouseLeave }) => ( - - - - {children} - - + (end - start) * nodeListRowHeight} + total={items.length} + onChange={({ start, end, total }) => + console.log( + `${group.name} ${start} to ${end} (${end - start} of ${total})` + ) + }> + {({ + start, + end, + total, + listRef, + upperRef, + lowerRef, + listStyle, + upperStyle, + lowerStyle + }) => ( + + )} + + ); export default NodeListGroup; diff --git a/src/components/node-list/node-list-groups.js b/src/components/node-list/node-list-groups.js index 40ba831c9e..31706557ee 100644 --- a/src/components/node-list/node-list-groups.js +++ b/src/components/node-list/node-list-groups.js @@ -1,7 +1,6 @@ import React, { useState } from 'react'; import { loadState, saveState } from '../../store/helpers'; import NodeListGroup from './node-list-group'; -import NodeListRow from './node-list-row'; const storedState = loadState(); @@ -35,9 +34,8 @@ const NodeListGroups = ({ const group = groups[typeId]; return ( - {(items[group.id] || []).map(item => ( - onItemClick(item)} - onMouseEnter={() => onItemMouseEnter(item)} - onMouseLeave={() => onItemMouseLeave(item)} - onChange={e => onItemChange(item, !e.target.checked)} - /> - ))} - + onToggleChecked={onToggleGroupChecked} + onItemClick={onItemClick} + onItemChange={onItemChange} + onItemMouseEnter={onItemMouseEnter} + onItemMouseLeave={onItemMouseLeave} + /> ); })} From e191e940c2f44b5619039f3f49f3104d54c0fd5f Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Fri, 13 Nov 2020 18:23:35 +0000 Subject: [PATCH 04/20] [KED-1923] Add fade effect to placeholders on lazy lists --- src/components/node-list/styles/_group.scss | 34 +++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/components/node-list/styles/_group.scss b/src/components/node-list/styles/_group.scss index 012c5857c3..cfd216d474 100644 --- a/src/components/node-list/styles/_group.scss +++ b/src/components/node-list/styles/_group.scss @@ -16,12 +16,46 @@ } .pipeline-nodelist__children { + // Avoid placeholder fade leaking out for small lists + overflow: hidden; + &--closed { display: none; } } } +$placeholder-fade: 120px; + +.pipeline-nodelist__placeholder-upper, +.pipeline-nodelist__placeholder-lower { + z-index: 2; +} + +.pipeline-nodelist__placeholder-upper:after, +.pipeline-nodelist__placeholder-lower:after { + position: absolute; + bottom: -$placeholder-fade; + width: 100%; + height: $placeholder-fade; + background: linear-gradient(0deg, transparent 0%, #f0f1f3 100%); + opacity: 0; + transition: opacity ease 0.3s; + content: ' '; + pointer-events: none; +} + +.pipeline-nodelist__placeholder-lower:after { + top: -$placeholder-fade; + bottom: initial; + background: linear-gradient(0deg, #f0f1f3 0%, transparent 100%); +} + +.pipeline-nodelist__placeholder-upper--fade:after, +.pipeline-nodelist__placeholder-lower--fade:after { + opacity: 1; +} + .pipeline-nodelist__heading { margin: 0; From 8cdba338ff8dd2b0715d355fbe6e7927c4ccc5c7 Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Tue, 17 Nov 2020 16:50:36 +0000 Subject: [PATCH 05/20] [KED-1923] Add comments and docs to LazyList --- src/components/lazy-list/index.js | 180 +++++++++++++++++++++++++----- 1 file changed, 152 insertions(+), 28 deletions(-) diff --git a/src/components/lazy-list/index.js b/src/components/lazy-list/index.js index 64c9e9baf1..271d2bb29f 100644 --- a/src/components/lazy-list/index.js +++ b/src/components/lazy-list/index.js @@ -1,37 +1,67 @@ import { useState, useLayoutEffect, useRef, useMemo, useCallback } from 'react'; +// Required feature checks const supported = typeof IntersectionObserver !== 'undefined'; +/** + * A component that renders only the children currently visible on screen. + * @param {function} height A `function(start, end)` returning the pixel height for any given range of items + * @param {number} total The total count of all items in the list + * @param {function} children A `function(props)` rendering the list and items (see `childProps`) + * @param {?number} [buffer=0.5] A number [0...1] as a % of the visible region to render additionally + * @param {?boolean} [lazy=true] Toggles the lazy functionality + * @param {?boolean} [dispose=false] Toggles disposing items when they lose visibility + * @param {?function} onChange Optional change callback + * @return {object} The rendered children + **/ export default ({ - children, height, total, - onChange, + children, lazy = true, dispose = false, - buffer = 0.5 + buffer = 0.5, + onChange }) => { + // Active only if enabled by prop and features detected const active = lazy && supported; + // The range of items currently rendered const [range, setRange] = useState([0, 0]); - const nextRange = useRef([0, 0]); + const rangeRef = useRef([0, 0]); + // List container element const listRef = useRef(); - const lowerRef = useRef(); + + // Upper placeholder element const upperRef = useRef(); + // Lower placeholder element + const lowerRef = useRef(); + + // Height of a single item const itemHeight = useMemo(() => height(0, 1), [height]); + + // Height of all items const totalHeight = useMemo(() => height(0, total), [height, total]); + + // Height of items above the rendered range const upperHeight = useMemo(() => height(0, range[0]), [height, range]); + + // Height of items below the rendered range const lowerHeight = useMemo(() => height(range[1], total), [ height, range, total ]); + // Skipped if not enabled or supported if (active) { + // Allows an update only once per frame const requestUpdate = useRequestFrameOnce( + // Memoise the frame callback useCallback(() => { + // Get the range of items visible in this frame const visibleRange = visibleRangeOf( listRef.current, listRef.current?.offsetParent, @@ -40,40 +70,63 @@ export default ({ itemHeight ); - const effectiveRange = dispose - ? visibleRange - : rangeUnion(nextRange.current, visibleRange); + // Merge ranges + const effectiveRange = + // If dispose, render visible range only + dispose + ? visibleRange + : // If not dispose, expand current range with visible range + rangeUnion(rangeRef.current, visibleRange); - // Avoid duplicate renders if ranges are equal - if (!rangeEqual(nextRange.current, effectiveRange)) { - nextRange.current = effectiveRange; + // Avoid duplicate render calls as state is not set immediate + if (!rangeEqual(rangeRef.current, effectiveRange)) { + // Store the update in a ref immediately + rangeRef.current = effectiveRange; + + // Apply the update in the next render setRange(effectiveRange); } }, [buffer, total, itemHeight, dispose]) ); - const observerOptions = useMemo(() => ({ threshold: thresholds(total) }), [ - total - ]); + // Memoised observer options + const observerOptions = useMemo( + () => ({ + // Create a threshold point for every item + threshold: thresholds(total) + }), + [total] + ); + // Updates on changes in visibility at the given thresholds (intersection ratios) useIntersection(listRef, observerOptions, requestUpdate); useIntersection(upperRef, observerOptions, requestUpdate); useIntersection(lowerRef, observerOptions, requestUpdate); - useLayoutEffect(() => requestUpdate(), [total, totalHeight, requestUpdate]); + // Updates on changes in item dimensions + useLayoutEffect(() => requestUpdate(), [ + total, + itemHeight, + totalHeight, + requestUpdate + ]); } + // Memoised child props for user to apply as needed const childProps = useMemo( () => ({ - start: active ? range[0] : 0, - end: active ? range[1] : total, - total, listRef, upperRef, lowerRef, + total, + start: active ? range[0] : 0, + end: active ? range[1] : total, listStyle: { + // Relative for placeholder positioning position: 'relative', + // List must always have the correct height height: active ? totalHeight : undefined, + // List must always pad missing items (upper at least) paddingTop: active ? upperHeight : undefined }, upperStyle: { @@ -81,6 +134,7 @@ export default ({ display: !active ? 'none' : undefined, height: upperHeight, width: '100%', + // Upper placeholder must always snap to top edge top: '0' }, lowerStyle: { @@ -88,6 +142,7 @@ export default ({ display: !active ? 'none' : undefined, height: lowerHeight, width: '100%', + // Lower placeholder must always snap to bottom edge bottom: '0' } }), @@ -104,77 +159,146 @@ export default ({ ] ); + // Optional change callback onChange && onChange(childProps); + // Render the children return children(childProps); }; +/** + * Returns a range in the form `[start, end]` clamped inside `[min, max]` + * @param {number} start The start of the range + * @param {number} end The end of the range + * @param {number} min The range minimum + * @param {number} max The range maximum + * @returns {array} The clamped range + */ const range = (start, end, min, max) => [ Math.max(Math.min(start, max), min), Math.max(Math.min(end, max), min) ]; +/** + * Returns the union of both ranges + * @param {array} rangeA The first range `[start, end]` + * @param {array} rangeB The second range `[start, end]` + * @returns {array} The range union + */ const rangeUnion = (rangeA, rangeB) => [ Math.min(rangeA[0], rangeB[0]), Math.max(rangeA[1], rangeB[1]) ]; +/** + * Returns true if the ranges have the same `start` and `end` values + * @param {array} rangeA The first range `[start, end]` + * @param {array} rangeB The second range `[start, end]` + * @returns {boolean} True if ranges are equal else false + */ const rangeEqual = (rangeA, rangeB) => rangeA[0] === rangeB[0] && rangeA[1] === rangeB[1]; -const visibleRangeOf = (element, container, buffer, total, itemHeight) => { +/** + * Gets the range of items inside the container's screen bounds + * @param {HTMLElement} element The target element (e.g. the list element) + * @param {HTMLElement} container The container of the target (e.g. a scrolling element) + * @param {number} buffer A number [0...1] as a % of the container to render additionally + * @param {number} childTotal The total count of all children in the target (e.g. the list rows) + * @param {number} childHeight Height of a single child element (e.g. a single list row) + * @returns {array} The calculated range of visible items as `[start, end]` + */ +const visibleRangeOf = ( + element, + container, + buffer, + childTotal, + childHeight +) => { + // Check both elements exist if (!element || !container) { return [0, 0]; } + // Find element bounds const rect = element.getBoundingClientRect(); const clip = container.getBoundingClientRect(); - const bufferCount = Math.ceil((buffer * clip.height) / itemHeight); + // Find the number of items to buffer + const bufferCount = Math.ceil((buffer * clip.height) / childHeight); + // When element is fully above the container if (rect.bottom < clip.top) { - return range(total - bufferCount, total, 0, total); + return range(childTotal - bufferCount, childTotal, 0, childTotal); } + // When element is fully below the container if (rect.top > clip.bottom) { - return range(0, bufferCount, 0, total); + return range(0, bufferCount, 0, childTotal); } + // Find visible bounds by clipping element bounds on container bounds const top = Math.min(Math.max(rect.top, clip.top), clip.bottom); const bottom = Math.max(Math.min(rect.bottom, clip.bottom), clip.top); - const start = Math.floor((top - rect.top) / itemHeight); - const end = Math.ceil((bottom - rect.top) / itemHeight); + // Find the visible item range inside the visible bounds + const start = Math.floor((top - rect.top) / childHeight); + const end = Math.ceil((bottom - rect.top) / childHeight); - return range(start - bufferCount, end + bufferCount, 0, total); + // Apply buffer and clamp final range + return range(start - bufferCount, end + bufferCount, 0, childTotal); }; +/** + * A hook to create a callback that runs once, at the end of the frame + * @param {function} callback The callback + * @returns {function} The wrapped callback + */ const useRequestFrameOnce = callback => { const request = useRef(); - // Keep the callback cached to avoid constant re-creation + // Allow only a single callback per-frame return useCallback(() => { cancelAnimationFrame(request.current); request.current = requestAnimationFrame(callback); }, [request, callback]); }; +/** + * Generates an array of the form [0, {i / total}, ..., 1] + * except where total is `0` where it returns `[0]`. + * @param {number} total The total number of thresholds to create + * @returns {array} The threshold array + */ const thresholds = total => - Array.from({ length: total }, (_, i) => i / (total - 1)); + total === 0 ? [0] : Array.from({ length: total }, (_, i) => i / (total - 1)); +/** + * A hook that creates and manages an IntersectionObserver for the given element + * @param {object} element A React.Ref from the target element + * @param {object} options An IntersectionObserver options object + * @param {function} callback A function to call with IntersectionObserver changes + */ const useIntersection = (element, options, callback) => { const observer = useRef(); - // Must be an effect to avoid constant re-creation + // After rendering and layout return useLayoutEffect(() => { + // Check the element is ready if (!element.current) { return; } + + // Dispose any previous observer if (observer.current) { observer.current.disconnect(); } + + // Create a new observer observer.current = new IntersectionObserver(callback, options); observer.current.observe(element.current); + + // Manually callback as element may already be visible callback(); }, [callback, element, options]); }; From ade568cd4af58d7d5292cc5849c5c88b686e455a Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Tue, 17 Nov 2020 18:07:23 +0000 Subject: [PATCH 06/20] [KED-1923] Fix placeholder fade on node list --- src/components/node-list/node-list-group.js | 5 +-- src/components/node-list/styles/_group.scss | 41 +++++++++++++++++++-- 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/src/components/node-list/node-list-group.js b/src/components/node-list/node-list-group.js index ae84e527b3..9d3aa4d0d6 100644 --- a/src/components/node-list/node-list-group.js +++ b/src/components/node-list/node-list-group.js @@ -56,7 +56,6 @@ export const NodeListGroup = ({ (end - start) * nodeListRowHeight} total={items.length} onChange={({ start, end, total }) => @@ -85,14 +84,14 @@ export const NodeListGroup = ({ )}>
  • 0 + fade: start !== end && start > 0 })} ref={upperRef} style={upperStyle} />
  • Date: Wed, 18 Nov 2020 12:06:51 +0000 Subject: [PATCH 07/20] [KED-1923] Add viewport clipping to LazyList --- src/components/lazy-list/index.js | 60 ++++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 17 deletions(-) diff --git a/src/components/lazy-list/index.js b/src/components/lazy-list/index.js index 271d2bb29f..eae3ba69ae 100644 --- a/src/components/lazy-list/index.js +++ b/src/components/lazy-list/index.js @@ -63,7 +63,9 @@ export default ({ useCallback(() => { // Get the range of items visible in this frame const visibleRange = visibleRangeOf( + // The list container listRef.current, + // The list's scrolling parent container listRef.current?.offsetParent, buffer, total, @@ -200,12 +202,13 @@ const rangeEqual = (rangeA, rangeB) => rangeA[0] === rangeB[0] && rangeA[1] === rangeB[1]; /** - * Gets the range of items inside the container's screen bounds - * @param {HTMLElement} element The target element (e.g. the list element) - * @param {HTMLElement} container The container of the target (e.g. a scrolling element) + * Gets the range of items inside the container's screen bounds. + * Assumes a single fixed height for all child items. + * @param {HTMLElement} element The target element (e.g. list container) + * @param {?HTMLElement} container The clipping container of the target (e.g. scroll container) * @param {number} buffer A number [0...1] as a % of the container to render additionally - * @param {number} childTotal The total count of all children in the target (e.g. the list rows) - * @param {number} childHeight Height of a single child element (e.g. a single list row) + * @param {number} childTotal The total count of all children in the target (e.g. list row count) + * @param {number} childHeight Height of a single child element (e.g. height of one list row) * @returns {array} The calculated range of visible items as `[start, end]` */ const visibleRangeOf = ( @@ -215,33 +218,56 @@ const visibleRangeOf = ( childTotal, childHeight ) => { - // Check both elements exist - if (!element || !container) { + // Check element exists + if (!element) { return [0, 0]; } - // Find element bounds - const rect = element.getBoundingClientRect(); + // If no container use the element itself + if (!container) { + container = element; + } + + // Find the clipping container bounds (e.g. scroll container) const clip = container.getBoundingClientRect(); + // Find element bounds (e.g. list container inside scroll container) + const rect = element.getBoundingClientRect(); + // Find the number of items to buffer const bufferCount = Math.ceil((buffer * clip.height) / childHeight); - // When element is fully above the container - if (rect.bottom < clip.top) { + // When clip is fully above viewport or element is fully above clip + if (clip.bottom < 0 || rect.bottom < clip.top) { + // Only bottom part of the buffer in range return range(childTotal - bufferCount, childTotal, 0, childTotal); } - // When element is fully below the container - if (rect.top > clip.bottom) { + // Get the viewport bounds + const viewport = { + top: 0, + bottom: window.innerHeight || document.documentElement.clientHeight + }; + + // When clip is fully below viewport or element is fully below clip + if (clip.top > viewport.bottom || rect.top > clip.bottom) { + // Only top part of the buffer in range return range(0, bufferCount, 0, childTotal); } - // Find visible bounds by clipping element bounds on container bounds - const top = Math.min(Math.max(rect.top, clip.top), clip.bottom); - const bottom = Math.max(Math.min(rect.bottom, clip.bottom), clip.top); + // Find visible rendered bounds inside scroll clip and inside viewport clip + const top = Math.min( + Math.max(rect.top, clip.top, viewport.top), + clip.bottom, + viewport.bottom + ); + const bottom = Math.max( + Math.min(rect.bottom, clip.bottom, viewport.bottom), + clip.top, + viewport.bottom + ); - // Find the visible item range inside the visible bounds + // Find visible item range inside visible rendered bounds const start = Math.floor((top - rect.top) / childHeight); const end = Math.ceil((bottom - rect.top) / childHeight); From eaa61a6b9d479a376db4d951c7b5e40d7bd21760 Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Wed, 18 Nov 2020 12:18:34 +0000 Subject: [PATCH 08/20] [KED-1923] Refactor into NodeRowList --- src/components/node-list/node-list-group.js | 83 +++--------------- .../node-list/node-list-row-list.js | 86 +++++++++++++++++++ src/components/node-list/node-list-row.js | 5 ++ 3 files changed, 102 insertions(+), 72 deletions(-) create mode 100644 src/components/node-list/node-list-row-list.js diff --git a/src/components/node-list/node-list-group.js b/src/components/node-list/node-list-group.js index 9d3aa4d0d6..8badccd942 100644 --- a/src/components/node-list/node-list-group.js +++ b/src/components/node-list/node-list-group.js @@ -1,8 +1,7 @@ import React from 'react'; import classnames from 'classnames'; -import modifiers from '../../utils/modifiers'; -import NodeListRow, { nodeListRowHeight } from './node-list-row'; -import LazyList from '../lazy-list'; +import NodeListRow from './node-list-row'; +import NodeRowList from './node-list-row-list'; export const NodeListGroup = ({ items, @@ -55,75 +54,15 @@ export const NodeListGroup = ({ /> - (end - start) * nodeListRowHeight} - total={items.length} - onChange={({ start, end, total }) => - console.log( - `${group.name} ${start} to ${end} (${end - start} of ${total})` - ) - }> - {({ - start, - end, - total, - listRef, - upperRef, - lowerRef, - listStyle, - upperStyle, - lowerStyle - }) => ( -
      -
    • 0 - })} - ref={upperRef} - style={upperStyle} - /> -
    • - {items.slice(start, end).map(item => ( - onItemClick(item)} - // Disabled to avoid unrelated hover lag for now. - // onMouseEnter={() => onItemMouseEnter(item)} - // onMouseLeave={() => onItemMouseLeave(item)} - onChange={e => onItemChange(item, !e.target.checked)} - /> - ))} -
    - )} -     +
    • ); diff --git a/src/components/node-list/node-list-row-list.js b/src/components/node-list/node-list-row-list.js new file mode 100644 index 0000000000..73f55b3533 --- /dev/null +++ b/src/components/node-list/node-list-row-list.js @@ -0,0 +1,86 @@ +import React from 'react'; +import modifiers from '../../utils/modifiers'; +import NodeListRow, { nodeListRowHeight } from './node-list-row'; +import LazyList from '../lazy-list'; + +export const NodeRowList = ({ + items, + group, + collapsed, + onItemClick, + onItemChange, + onItemMouseEnter, + onItemMouseLeave +}) => ( + (end - start) * nodeListRowHeight} + total={items.length} + onChange={({ start, end, total }) => + console.log( + `${group.name} ${start} to ${end} (${end - start} of ${total})` + ) + }> + {({ + start, + end, + total, + listRef, + upperRef, + lowerRef, + listStyle, + upperStyle, + lowerStyle + }) => ( + + )} + +); + +export default NodeRowList; diff --git a/src/components/node-list/node-list-row.js b/src/components/node-list/node-list-row.js index ae7ab32e13..7343b35d4c 100644 --- a/src/components/node-list/node-list-row.js +++ b/src/components/node-list/node-list-row.js @@ -7,8 +7,13 @@ import VisibleIcon from '../icons/visible'; import InvisibleIcon from '../icons/invisible'; import { getNodeActive } from '../../selectors/nodes'; +// The exact fixed height of a row export const nodeListRowHeight = 36.59375; +/** + * Returns `true` if there are no props changes, therefore the last render can be reused. + * Performance: Checks only the minimal set of props known to change after first render. + */ const shouldMemo = (prevProps, nextProps) => !changed( [ From 092cd69432ca08ffdaedbb7c0633b0f8d161b7e5 Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Wed, 18 Nov 2020 12:33:40 +0000 Subject: [PATCH 09/20] [KED-1923] Add notes on row height --- src/components/node-list/node-list-row.js | 2 +- src/components/node-list/styles/_row.scss | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/components/node-list/node-list-row.js b/src/components/node-list/node-list-row.js index 7343b35d4c..f77d910992 100644 --- a/src/components/node-list/node-list-row.js +++ b/src/components/node-list/node-list-row.js @@ -7,7 +7,7 @@ import VisibleIcon from '../icons/visible'; import InvisibleIcon from '../icons/invisible'; import { getNodeActive } from '../../selectors/nodes'; -// The exact fixed height of a row +// The exact fixed height of a row as measured by getBoundingClientRect() export const nodeListRowHeight = 36.59375; /** diff --git a/src/components/node-list/styles/_row.scss b/src/components/node-list/styles/_row.scss index b0ddf040b3..6cb1258de8 100644 --- a/src/components/node-list/styles/_row.scss +++ b/src/components/node-list/styles/_row.scss @@ -1,3 +1,11 @@ +/* + + Notes: + - any change to row height must be reflected here: + `nodeListRowHeight` in node-list-row.js + +*/ + .pipeline-nodelist__row { position: relative; display: flex; From 45e9ec2e4aa8aa72e9c408f98a63d4c898f323a7 Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Wed, 18 Nov 2020 15:14:48 +0000 Subject: [PATCH 10/20] [KED-1923] Add flag for lazy sidebar features --- README.md | 1 + .../node-list/node-list-row-list.js | 59 +++++++++++++++++-- src/config.js | 5 ++ src/selectors/linked-nodes.js | 6 +- 4 files changed, 64 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 678d53a637..558d294a2f 100644 --- a/README.md +++ b/README.md @@ -104,6 +104,7 @@ The following flags are available to toggle experimental features: - `newgraph` - From release v3.4.0. Improved graphing algorithm. (default `false`) - `meta` - From release v3.7.0. Show node metadata panel on click. (default `false`) +- `lazy` - From release vx.x.x. Improved sidebar performance. (default `false`) ### Setting flags diff --git a/src/components/node-list/node-list-row-list.js b/src/components/node-list/node-list-row-list.js index 73f55b3533..525d678f96 100644 --- a/src/components/node-list/node-list-row-list.js +++ b/src/components/node-list/node-list-row-list.js @@ -1,9 +1,52 @@ import React from 'react'; import modifiers from '../../utils/modifiers'; +import { connect } from 'react-redux'; import NodeListRow, { nodeListRowHeight } from './node-list-row'; import LazyList from '../lazy-list'; -export const NodeRowList = ({ +const NodeRowList = ({ + items, + group, + collapsed, + onItemClick, + onItemChange, + onItemMouseEnter, + onItemMouseLeave +}) => ( + +); + +const NodeRowLazyList = ({ items, group, collapsed, @@ -72,9 +115,8 @@ export const NodeRowList = ({ visibleIcon={item.visibleIcon} invisibleIcon={item.invisibleIcon} onClick={() => onItemClick(item)} - // Disabled to avoid unrelated hover lag for now. - // onMouseEnter={() => onItemMouseEnter(item)} - // onMouseLeave={() => onItemMouseLeave(item)} + onMouseEnter={() => onItemMouseEnter(item)} + onMouseLeave={() => onItemMouseLeave(item)} onChange={e => onItemChange(item, !e.target.checked)} /> ))} @@ -83,4 +125,11 @@ export const NodeRowList = ({     ); -export default NodeRowList; +export const mapStateToProps = (state, ownProps) => ({ + lazy: state.flags.lazy, + ...ownProps +}); + +export default connect(mapStateToProps)(props => + props.lazy ? : +); diff --git a/src/config.js b/src/config.js index 189ee7ca01..78140ace93 100644 --- a/src/config.js +++ b/src/config.js @@ -29,6 +29,11 @@ export const flags = { description: 'Show the metadata panel', default: false, icon: '🔮' + }, + lazy: { + description: 'Improved sidebar performance', + default: false, + icon: '😴' } }; diff --git a/src/selectors/linked-nodes.js b/src/selectors/linked-nodes.js index 802b316d2b..03e8740d7a 100644 --- a/src/selectors/linked-nodes.js +++ b/src/selectors/linked-nodes.js @@ -3,6 +3,7 @@ import { getVisibleEdges } from './edges'; const getClickedNode = state => state.node.clicked; const getHoveredNode = state => state.node.hovered; +const getLazyFlag = state => state.flags.lazy; /** * Get the node that should be used as the center of the set of linked nodes @@ -10,8 +11,9 @@ const getHoveredNode = state => state.node.hovered; * @param {string} nodeID */ export const getCentralNode = createSelector( - [getClickedNode, getHoveredNode], - (clickedNode, hoveredNode) => clickedNode || hoveredNode + [getClickedNode, getHoveredNode, getLazyFlag], + (clickedNode, hoveredNode, lazy) => + lazy ? clickedNode : clickedNode || hoveredNode ); /** From e2b3b1bc967661bcb410ff2608fbd70176978097 Mon Sep 17 00:00:00 2001 From: Richard Westenra Date: Thu, 26 Nov 2020 15:10:57 +0000 Subject: [PATCH 11/20] Use CSS custom properties for gradient theming --- src/components/node-list/styles/_group.scss | 44 ++++++--------------- 1 file changed, 12 insertions(+), 32 deletions(-) diff --git a/src/components/node-list/styles/_group.scss b/src/components/node-list/styles/_group.scss index 191367635b..73ce184b5e 100644 --- a/src/components/node-list/styles/_group.scss +++ b/src/components/node-list/styles/_group.scss @@ -1,3 +1,5 @@ +@import '../../../styles/variables'; + %nolist { margin: 0; padding: 0; @@ -46,42 +48,20 @@ $placeholder-fade: 120px; .pipeline-nodelist__placeholder-upper:after { bottom: -$placeholder-fade; - - .kui-theme--dark & { - background: linear-gradient( - 0deg, - rgba($color-bg-dark-3, 0) 0%, - rgba($color-bg-dark-3, 1) 100% - ); - } - - .kui-theme--light & { - background: linear-gradient( - 0deg, - rgba($color-bg-light-3, 0) 0%, - rgba($color-bg-light-3, 1) 100% - ); - } + background: linear-gradient( + 0deg, + var(--nodelist-bg-transparent) 0%, + var(--color-bg-3) 100% + ); } .pipeline-nodelist__placeholder-lower:after { top: -$placeholder-fade; - - .kui-theme--dark & { - background: linear-gradient( - 0deg, - rgba($color-bg-dark-3, 1) 0%, - rgba($color-bg-dark-3, 0) 100% - ); - } - - .kui-theme--light & { - background: linear-gradient( - 0deg, - rgba($color-bg-light-3, 1) 0%, - rgba($color-bg-light-3, 0) 100% - ); - } + background: linear-gradient( + 0deg, + var(--color-bg-3) 0%, + var(--nodelist-bg-transparent) 100% + ); } .pipeline-nodelist__placeholder-upper--fade:after, From 4b4d34b1d560d65bb54319ae3db01619174ddb28 Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Thu, 3 Dec 2020 18:56:02 +0000 Subject: [PATCH 12/20] [KED-1923] Fix tests for node-list after refactor --- .../node-list/node-list-group.test.js | 27 ++++++++++--------- .../node-list/node-list-row-list.js | 15 ++++------- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/src/components/node-list/node-list-group.test.js b/src/components/node-list/node-list-group.test.js index adb9f1550c..1d3900b96a 100644 --- a/src/components/node-list/node-list-group.test.js +++ b/src/components/node-list/node-list-group.test.js @@ -1,5 +1,6 @@ import React from 'react'; import { NodeListGroup } from './node-list-group'; +import NodeListRow from './node-list-row'; import { getNodeTypes } from '../../selectors/node-types'; import { setup, mockState } from '../../utils/state.mock'; @@ -11,16 +12,6 @@ describe('NodeListGroup', () => { ).not.toThrow(); }); - it('renders children', () => { - const type = getNodeTypes(mockState.animals)[0]; - const wrapper = setup.mount( - -
    - - ); - expect(wrapper.find('.test-child').length).toBe(1); - }); - it('handles checkbox change events', () => { const type = getNodeTypes(mockState.animals)[0]; const onToggleChecked = jest.fn(); @@ -50,11 +41,23 @@ describe('NodeListGroup', () => { expect(onToggleCollapsed.mock.calls.length).toEqual(1); }); - it('hides children when collapsed class is used', () => { + it('adds class when collapsed prop true', () => { const type = getNodeTypes(mockState.animals)[0]; const wrapper = setup.mount( ); - expect(wrapper.find('.pipeline-nodelist__list--nested').length).toEqual(0); + const children = wrapper.find('.pipeline-nodelist__children'); + expect(children.hasClass('pipeline-nodelist__children--closed')).toBe(true); + }); + + it('removes class when collapsed prop false', () => { + const type = getNodeTypes(mockState.animals)[0]; + const wrapper = setup.mount( + + ); + const children = wrapper.find('.pipeline-nodelist__children'); + expect(children.hasClass('pipeline-nodelist__children--closed')).toBe( + false + ); }); }); diff --git a/src/components/node-list/node-list-row-list.js b/src/components/node-list/node-list-row-list.js index 525d678f96..59781ca233 100644 --- a/src/components/node-list/node-list-row-list.js +++ b/src/components/node-list/node-list-row-list.js @@ -5,7 +5,7 @@ import NodeListRow, { nodeListRowHeight } from './node-list-row'; import LazyList from '../lazy-list'; const NodeRowList = ({ - items, + items = [], group, collapsed, onItemClick, @@ -17,7 +17,7 @@ const NodeRowList = ({ className={modifiers( 'pipeline-nodelist__children', { closed: collapsed }, - 'pipeline-nodelist__list' + 'pipeline-nodelist__list pipeline-nodelist__list--nested' )}> {items.map(item => ( ( (end - start) * nodeListRowHeight} - total={items.length} - onChange={({ start, end, total }) => - console.log( - `${group.name} ${start} to ${end} (${end - start} of ${total})` - ) - }> + total={items.length}> {({ start, end, @@ -80,7 +75,7 @@ const NodeRowLazyList = ({ className={modifiers( 'pipeline-nodelist__children', { closed: collapsed }, - 'pipeline-nodelist__list' + 'pipeline-nodelist__list pipeline-nodelist__list--nested' )}>
  • Date: Thu, 3 Dec 2020 18:57:12 +0000 Subject: [PATCH 13/20] [KED-1923] Add tests for LazyList --- src/components/lazy-list/index.js | 31 ++-- src/components/lazy-list/lazy-list.test.js | 197 +++++++++++++++++++++ 2 files changed, 215 insertions(+), 13 deletions(-) create mode 100644 src/components/lazy-list/lazy-list.test.js diff --git a/src/components/lazy-list/index.js b/src/components/lazy-list/index.js index eae3ba69ae..93dda8facf 100644 --- a/src/components/lazy-list/index.js +++ b/src/components/lazy-list/index.js @@ -1,8 +1,5 @@ import { useState, useLayoutEffect, useRef, useMemo, useCallback } from 'react'; -// Required feature checks -const supported = typeof IntersectionObserver !== 'undefined'; - /** * A component that renders only the children currently visible on screen. * @param {function} height A `function(start, end)` returning the pixel height for any given range of items @@ -12,6 +9,7 @@ const supported = typeof IntersectionObserver !== 'undefined'; * @param {?boolean} [lazy=true] Toggles the lazy functionality * @param {?boolean} [dispose=false] Toggles disposing items when they lose visibility * @param {?function} onChange Optional change callback + * @param {?function} container Optional, default scroll container is `element.offsetParent` * @return {object} The rendered children **/ export default ({ @@ -21,8 +19,12 @@ export default ({ lazy = true, dispose = false, buffer = 0.5, - onChange + onChange, + container = element => element?.offsetParent }) => { + // Required feature checks + const supported = typeof window.IntersectionObserver !== 'undefined'; + // Active only if enabled by prop and features detected const active = lazy && supported; @@ -66,7 +68,7 @@ export default ({ // The list container listRef.current, // The list's scrolling parent container - listRef.current?.offsetParent, + container(listRef.current), buffer, total, itemHeight @@ -88,7 +90,7 @@ export default ({ // Apply the update in the next render setRange(effectiveRange); } - }, [buffer, total, itemHeight, dispose]) + }, [buffer, total, itemHeight, dispose, container]) ); // Memoised observer options @@ -176,7 +178,7 @@ export default ({ * @param {number} max The range maximum * @returns {array} The clamped range */ -const range = (start, end, min, max) => [ +export const range = (start, end, min, max) => [ Math.max(Math.min(start, max), min), Math.max(Math.min(end, max), min) ]; @@ -187,7 +189,7 @@ const range = (start, end, min, max) => [ * @param {array} rangeB The second range `[start, end]` * @returns {array} The range union */ -const rangeUnion = (rangeA, rangeB) => [ +export const rangeUnion = (rangeA, rangeB) => [ Math.min(rangeA[0], rangeB[0]), Math.max(rangeA[1], rangeB[1]) ]; @@ -198,12 +200,13 @@ const rangeUnion = (rangeA, rangeB) => [ * @param {array} rangeB The second range `[start, end]` * @returns {boolean} True if ranges are equal else false */ -const rangeEqual = (rangeA, rangeB) => +export const rangeEqual = (rangeA, rangeB) => rangeA[0] === rangeB[0] && rangeA[1] === rangeB[1]; /** * Gets the range of items inside the container's screen bounds. * Assumes a single fixed height for all child items. + * Only considers visibility along the vertical y-axis (i.e. only top, bottom bounds). * @param {HTMLElement} element The target element (e.g. list container) * @param {?HTMLElement} container The clipping container of the target (e.g. scroll container) * @param {number} buffer A number [0...1] as a % of the container to render additionally @@ -291,13 +294,15 @@ const useRequestFrameOnce = callback => { }; /** - * Generates an array of the form [0, {i / total}, ..., 1] + * Generates an array of the form [0, ...n / total] * except where total is `0` where it returns `[0]`. * @param {number} total The total number of thresholds to create * @returns {array} The threshold array */ -const thresholds = total => - total === 0 ? [0] : Array.from({ length: total }, (_, i) => i / (total - 1)); +export const thresholds = total => + total === 0 + ? [0] + : [...Array.from({ length: total }, (_, i) => i / total), 1]; /** * A hook that creates and manages an IntersectionObserver for the given element @@ -321,7 +326,7 @@ const useIntersection = (element, options, callback) => { } // Create a new observer - observer.current = new IntersectionObserver(callback, options); + observer.current = new window.IntersectionObserver(callback, options); observer.current.observe(element.current); // Manually callback as element may already be visible diff --git a/src/components/lazy-list/lazy-list.test.js b/src/components/lazy-list/lazy-list.test.js new file mode 100644 index 0000000000..fcd98172ee --- /dev/null +++ b/src/components/lazy-list/lazy-list.test.js @@ -0,0 +1,197 @@ +import React from 'react'; +import LazyList, { range, rangeUnion, rangeEqual, thresholds } from './index'; +import { setup } from '../../utils/state.mock'; + +describe('LazyList', () => { + it('renders expected visible child items with padding for non-visible items', () => { + const itemCount = 500; + const itemHeight = 30; + const visibleStart = 10; + const visibleEnd = 40; + const visibleCount = visibleEnd - visibleStart; + + const test = setupTest({ + itemCount, + itemHeight, + visibleCount, + containerHeight: itemHeight * visibleCount, + viewportHeight: itemHeight * visibleCount, // * 1.25, + containerScrollY: itemHeight * visibleStart * 0.5, + viewportScrollY: itemHeight * visibleStart * 0.5 + }); + + const wrapper = setup.mount( + element?.parentElement}> + {test.listRender} + + ); + + // Get rendered and expected children text + const childrenText = wrapper + .find('.test-child') + .map(element => element.text()); + const expectedChildrenText = Array.from( + { length: visibleCount }, + (_, i) => `Item ${visibleStart + i}` + ); + + // Sanity checks + expect(childrenText.length).toBe(visibleCount); + expect(childrenText.length).toBeLessThan(itemCount); + + // Test rendered children are strictly the expected visible children + expect(childrenText).toEqual(expectedChildrenText); + + // Test list pads the non-visible items + const listElementStyle = wrapper.find('.test-list').get(0).props.style; + expect(listElementStyle.paddingTop).toBe(visibleStart * itemHeight); + expect(listElementStyle.height).toBe(itemCount * itemHeight); + }); + + it('range(from, to, min, max) returns [max(from, min), min(to, max)]', () => { + expect(range(0, 1, 0, 1)).toEqual([0, 1]); + expect(range(-1, 1, 0, 1)).toEqual([0, 1]); + expect(range(-1, 2, 0, 1)).toEqual([0, 1]); + }); + + it('rangeUnion(a, b) returns [min(a[0], b[0]), max(a[1], b[1])]', () => { + expect(rangeUnion([3, 7], [2, 10])).toEqual([2, 10]); + expect(rangeUnion([2, 10], [3, 7])).toEqual([2, 10]); + expect(rangeUnion([1, 7], [2, 10])).toEqual([1, 10]); + expect(rangeUnion([3, 11], [2, 10])).toEqual([2, 11]); + expect(rangeUnion([1, 11], [2, 10])).toEqual([1, 11]); + }); + + it('rangeEqual(a, b) returns true if a[0] = b[0] && a[1] = b[1]', () => { + expect(rangeEqual([1, 2], [1, 2])).toBe(true); + expect(rangeEqual([1, 2], [1, 3])).toBe(false); + expect(rangeEqual([1, 2], [3, 1])).toBe(false); + }); + + it('thresholds(t) returns [0, ...n / t] except t = `0` returns `[0]`', () => { + expect(thresholds(0)).toEqual([0]); + expect(thresholds(1)).toEqual([0, 1]); + expect(thresholds(2)).toEqual([0, 1 / 2, 1]); + expect(thresholds(3)).toEqual([0, 1 / 3, 2 / 3, 1]); + expect(thresholds(4)).toEqual([0, 1 / 4, 2 / 4, 3 / 4, 1]); + }); +}); + +// Sets up the test data and environment +const setupTest = ({ + itemCount, + itemHeight, + visibleCount, + viewportHeight, + viewportScrollY, + containerHeight, + containerScrollY +}) => { + // Generate test data and settings + const items = Array.from({ length: itemCount }, (_, i) => i); + const itemHeights = (start, end) => (end - start) * itemHeight; + const itemWidth = itemHeight * 5; + const containerWidth = itemWidth; + const listWidth = itemWidth; + + // Children render function + const listRender = ({ + start, + end, + listRef, + upperRef, + lowerRef, + listStyle, + upperStyle, + lowerStyle + }) => ( + <> + {/* Scroll container */} +
    + {/* List container */} +
      + {/* Upper placeholder */} +
    • + {/* Lower placeholder */} +
    • + {/* List items subset */} + {items.slice(start, end).map(i => ( +
    • + Item {i} +
      • + ))} +
    +
    + + ); + + // Emulate the browser window viewport height + window.innerHeight = viewportHeight; + + // Emulate RAF with immediate callback + window.requestAnimationFrame = cb => cb(0); + + // Emulate IntersectionObserver with immediate callback + window.IntersectionObserver = function(cb) { + return { + observe: () => cb(), + disconnect: () => null + }; + }; + + // Gets the React instance for a React node + const getInstance = node => { + const key = Object.keys(node).find(key => + key.startsWith('__reactInternalInstance') + ); + return node[key]; + }; + + // Emulate element bounds as if rendered + window.Element.prototype.getBoundingClientRect = function() { + const instance = getInstance(this); + + // Check which element this is + const isList = instance.type === 'ul'; + + // Set by `style` in `listRender` + const width = Number.parseInt(this.style.width); + const height = Number.parseInt(this.style.height); + + // Find offset for viewport scroll and container scroll + const offsetY = -viewportScrollY - (isList ? containerScrollY : 0); + + // Return bounds in screen space as expected + return { + x: 0, + left: 0, + y: offsetY, + top: offsetY, + width: width, + right: width, + height: height, + bottom: height + offsetY + }; + }; + + // Return the test setup + return { + items, + visibleCount, + itemHeights, + listRender + }; +}; From a77e7014625888ef0df68f59e07c85eff5fab8e3 Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Mon, 7 Dec 2020 13:24:31 +0000 Subject: [PATCH 14/20] [KED-1923] Simplify LazyList clipping --- src/components/lazy-list/index.js | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/src/components/lazy-list/index.js b/src/components/lazy-list/index.js index 93dda8facf..ee0f9a3ca5 100644 --- a/src/components/lazy-list/index.js +++ b/src/components/lazy-list/index.js @@ -235,13 +235,13 @@ const visibleRangeOf = ( const clip = container.getBoundingClientRect(); // Find element bounds (e.g. list container inside scroll container) - const rect = element.getBoundingClientRect(); + const list = element.getBoundingClientRect(); // Find the number of items to buffer const bufferCount = Math.ceil((buffer * clip.height) / childHeight); // When clip is fully above viewport or element is fully above clip - if (clip.bottom < 0 || rect.bottom < clip.top) { + if (clip.bottom < 0 || list.bottom < clip.top) { // Only bottom part of the buffer in range return range(childTotal - bufferCount, childTotal, 0, childTotal); } @@ -253,28 +253,20 @@ const visibleRangeOf = ( }; // When clip is fully below viewport or element is fully below clip - if (clip.top > viewport.bottom || rect.top > clip.bottom) { + if (clip.top > viewport.bottom || list.top > clip.bottom) { // Only top part of the buffer in range return range(0, bufferCount, 0, childTotal); } - // Find visible rendered bounds inside scroll clip and inside viewport clip - const top = Math.min( - Math.max(rect.top, clip.top, viewport.top), - clip.bottom, - viewport.bottom - ); - const bottom = Math.max( - Math.min(rect.bottom, clip.bottom, viewport.bottom), - clip.top, - viewport.bottom - ); + // Find intersection of clip and viewport now overlap guaranteed + const top = Math.max(clip.top, viewport.top); + const bottom = Math.min(clip.bottom, viewport.bottom); - // Find visible item range inside visible rendered bounds - const start = Math.floor((top - rect.top) / childHeight); - const end = Math.ceil((bottom - rect.top) / childHeight); + // Find unbounded item range within the intersection + const start = Math.floor((top - list.top) / childHeight); + const end = Math.ceil((bottom - list.top) / childHeight); - // Apply buffer and clamp final range + // Apply buffer and clamp unbounded range to list bounds return range(start - bufferCount, end + bufferCount, 0, childTotal); }; From 307e4e90d476aa8d5663faddef4199605959fee0 Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Mon, 7 Dec 2020 13:28:35 +0000 Subject: [PATCH 15/20] [KED-1923] Improve tests for LazyList --- src/components/lazy-list/lazy-list.test.js | 56 +++++++++++-------- .../node-list/node-list-group.test.js | 1 - 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/src/components/lazy-list/lazy-list.test.js b/src/components/lazy-list/lazy-list.test.js index fcd98172ee..7415ee1698 100644 --- a/src/components/lazy-list/lazy-list.test.js +++ b/src/components/lazy-list/lazy-list.test.js @@ -4,19 +4,27 @@ import { setup } from '../../utils/state.mock'; describe('LazyList', () => { it('renders expected visible child items with padding for non-visible items', () => { + // Settings for all test items const itemCount = 500; const itemHeight = 30; + + // The specific range of items to make visible in this test const visibleStart = 10; const visibleEnd = 40; const visibleCount = visibleEnd - visibleStart; + // Configure test to include some clipping and scroll conditions const test = setupTest({ itemCount, itemHeight, visibleCount, - containerHeight: itemHeight * visibleCount, - viewportHeight: itemHeight * visibleCount, // * 1.25, + // Viewport to fit exactly desired number of visible items + viewportHeight: itemHeight * visibleCount, + // Container larger than viewport to test clipping + containerHeight: itemHeight * visibleCount * 2, + // Container scrolled half way to desired start item to test containerScrollY: itemHeight * visibleStart * 0.5, + // Viewport scrolled remaining half way to desired start item viewportScrollY: itemHeight * visibleStart * 0.5 }); @@ -31,23 +39,25 @@ describe('LazyList', () => { ); - // Get rendered and expected children text - const childrenText = wrapper - .find('.test-child') - .map(element => element.text()); - const expectedChildrenText = Array.from( + // Generate expected items text for visible range + const expectedItemsText = Array.from( { length: visibleCount }, (_, i) => `Item ${visibleStart + i}` ); - // Sanity checks - expect(childrenText.length).toBe(visibleCount); - expect(childrenText.length).toBeLessThan(itemCount); + // Get actual rendered items text + const actualItemsText = wrapper + .find('.test-item') + .map(element => element.text()); + + // Test the items are exactly as expected + expect(actualItemsText).toEqual(expectedItemsText); - // Test rendered children are strictly the expected visible children - expect(childrenText).toEqual(expectedChildrenText); + // Sanity check that not all items were rendered + expect(actualItemsText.length).toBe(visibleCount); + expect(actualItemsText.length).toBeLessThan(itemCount); - // Test list pads the non-visible items + // Test element pads the remaining non-visible items const listElementStyle = wrapper.find('.test-list').get(0).props.style; expect(listElementStyle.paddingTop).toBe(visibleStart * itemHeight); expect(listElementStyle.height).toBe(itemCount * itemHeight); @@ -99,7 +109,7 @@ const setupTest = ({ const containerWidth = itemWidth; const listWidth = itemWidth; - // Children render function + // List render function const listRender = ({ start, end, @@ -127,9 +137,9 @@ const setupTest = ({
  • {/* Lower placeholder */}
  • - {/* List items subset */} + {/* List items in visible range */} {items.slice(start, end).map(i => ( -
  • +
  • Item {i}
    • ))} @@ -164,12 +174,12 @@ const setupTest = ({ window.Element.prototype.getBoundingClientRect = function() { const instance = getInstance(this); - // Check which element this is + // Check which element this is (list or container) const isList = instance.type === 'ul'; // Set by `style` in `listRender` - const width = Number.parseInt(this.style.width); - const height = Number.parseInt(this.style.height); + const width = Number.parseInt(this.style.width) || 0; + const height = Number.parseInt(this.style.height) || 0; // Find offset for viewport scroll and container scroll const offsetY = -viewportScrollY - (isList ? containerScrollY : 0); @@ -177,13 +187,13 @@ const setupTest = ({ // Return bounds in screen space as expected return { x: 0, - left: 0, y: offsetY, top: offsetY, - width: width, + bottom: offsetY + height, + left: 0, right: width, - height: height, - bottom: height + offsetY + width: width, + height: height }; }; diff --git a/src/components/node-list/node-list-group.test.js b/src/components/node-list/node-list-group.test.js index 1d3900b96a..7cb0b34480 100644 --- a/src/components/node-list/node-list-group.test.js +++ b/src/components/node-list/node-list-group.test.js @@ -1,6 +1,5 @@ import React from 'react'; import { NodeListGroup } from './node-list-group'; -import NodeListRow from './node-list-row'; import { getNodeTypes } from '../../selectors/node-types'; import { setup, mockState } from '../../utils/state.mock'; From e6c0643d19f8cfae67947c387ba90046e345b1b9 Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Tue, 8 Dec 2020 12:53:50 +0000 Subject: [PATCH 16/20] [KED-1923] Update version for lazy flag in readme --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 558d294a2f..070e2e9867 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,7 @@ The following flags are available to toggle experimental features: - `newgraph` - From release v3.4.0. Improved graphing algorithm. (default `false`) - `meta` - From release v3.7.0. Show node metadata panel on click. (default `false`) -- `lazy` - From release vx.x.x. Improved sidebar performance. (default `false`) +- `lazy` - From release v3.8.0. Improved sidebar performance. (default `false`) ### Setting flags From 9a38263370ebb4d4a98bfa98ab9da73cfea8f522 Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Fri, 11 Dec 2020 14:56:10 +0000 Subject: [PATCH 17/20] [KED-1923] Fix LazyList tests for CI --- src/components/lazy-list/lazy-list.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/lazy-list/lazy-list.test.js b/src/components/lazy-list/lazy-list.test.js index 7415ee1698..f1b20bbfc7 100644 --- a/src/components/lazy-list/lazy-list.test.js +++ b/src/components/lazy-list/lazy-list.test.js @@ -175,7 +175,7 @@ const setupTest = ({ const instance = getInstance(this); // Check which element this is (list or container) - const isList = instance.type === 'ul'; + const isList = instance?.type === 'ul'; // Set by `style` in `listRender` const width = Number.parseInt(this.style.width) || 0; From 17ac26d16f569e7df8b44223f6cb46a72d63ad4f Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Fri, 11 Dec 2020 15:26:52 +0000 Subject: [PATCH 18/20] [KED-1923] Fix app tests --- tools/test-lib/react-app/app.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/test-lib/react-app/app.test.js b/tools/test-lib/react-app/app.test.js index 027b52ce6d..6947807e2b 100644 --- a/tools/test-lib/react-app/app.test.js +++ b/tools/test-lib/react-app/app.test.js @@ -23,7 +23,7 @@ describe('lib-test', () => { const firstNodeName = wrapper .find('.pipeline-nodelist__group--type-task') .find('.pipeline-nodelist__list--nested') - .find('NodeListRow') + .find('.pipeline-nodelist__row__label') .first() .text(); if (key === 'random') { From b6d4b5b3d09f2d42521426b20dde93ae18ebd1a2 Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Mon, 14 Dec 2020 16:00:55 +0000 Subject: [PATCH 19/20] [KED-1923] Add comments to LazyList about behaviour when not supported --- src/components/lazy-list/index.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/components/lazy-list/index.js b/src/components/lazy-list/index.js index ee0f9a3ca5..5519d837c5 100644 --- a/src/components/lazy-list/index.js +++ b/src/components/lazy-list/index.js @@ -2,6 +2,7 @@ import { useState, useLayoutEffect, useRef, useMemo, useCallback } from 'react'; /** * A component that renders only the children currently visible on screen. + * Renders all children if not supported by browser or is disabled via the `lazy` prop. * @param {function} height A `function(start, end)` returning the pixel height for any given range of items * @param {number} total The total count of all items in the list * @param {function} children A `function(props)` rendering the list and items (see `childProps`) @@ -22,10 +23,10 @@ export default ({ onChange, container = element => element?.offsetParent }) => { - // Required feature checks + // Required browser feature checks const supported = typeof window.IntersectionObserver !== 'undefined'; - // Active only if enabled by prop and features detected + // Active only if supported and enabled else renders all children const active = lazy && supported; // The range of items currently rendered From 7ae380abc11170b01384560f1ab5956e82859475 Mon Sep 17 00:00:00 2001 From: bru5 <45365769+bru5@users.noreply.github.com> Date: Mon, 14 Dec 2020 16:01:55 +0000 Subject: [PATCH 20/20] [KED-1923] Move changed utility to shared module --- src/components/node-list/node-list-row.js | 2 +- src/utils/changed.js | 9 --------- src/utils/index.js | 14 ++++++++++++++ 3 files changed, 15 insertions(+), 10 deletions(-) delete mode 100644 src/utils/changed.js diff --git a/src/components/node-list/node-list-row.js b/src/components/node-list/node-list-row.js index f77d910992..a7e05cd0cb 100644 --- a/src/components/node-list/node-list-row.js +++ b/src/components/node-list/node-list-row.js @@ -1,7 +1,7 @@ import React, { memo } from 'react'; import { connect } from 'react-redux'; import classnames from 'classnames'; -import changed from '../../utils/changed'; +import { changed } from '../../utils'; import NodeIcon from '../icons/node-icon'; import VisibleIcon from '../icons/visible'; import InvisibleIcon from '../icons/invisible'; diff --git a/src/utils/changed.js b/src/utils/changed.js deleted file mode 100644 index 5861f92fb4..0000000000 --- a/src/utils/changed.js +++ /dev/null @@ -1,9 +0,0 @@ -/** - * Returns true if any of the given props are different between given objects. - * Only shallow changes are detected. - */ -export default (props, objectA, objectB) => { - return ( - objectA && objectB && props.some(prop => objectA[prop] !== objectB[prop]) - ); -}; diff --git a/src/utils/index.js b/src/utils/index.js index 4819377ef4..d428c05092 100644 --- a/src/utils/index.js +++ b/src/utils/index.js @@ -42,3 +42,17 @@ export const getUrl = (type, id) => { * @param {Array} arr The array to remove duplicate values from */ export const unique = (d, i, arr) => arr.indexOf(d) === i; + +/** + * Returns true if any of the given props are different between given objects. + * Only shallow changes are detected. + * @param {Array} props The prop names to check + * @param {object} objectA The first object + * @param {object} objectB The second object + * @returns {boolean} True if any prop changed else false + */ +export const changed = (props, objectA, objectB) => { + return ( + objectA && objectB && props.some(prop => objectA[prop] !== objectB[prop]) + ); +};