From ab4c09fde2dd361aef1aaf3b5395058d63e71e2c Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Sun, 28 Jul 2024 21:48:26 -0400 Subject: [PATCH 1/6] fix: dropdown does not flicker on firefox --- docs/CHANGELOG.md | 1 + src/app/dim-ui/PressTip.tsx | 1 + src/app/dim-ui/Select.tsx | 1 + src/app/dim-ui/destiny-symbols/SymbolsPicker.tsx | 1 + src/app/dim-ui/usePopper.ts | 10 +++++++++- 5 files changed, 13 insertions(+), 1 deletion(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index d5c6183701..f9495d98da 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -10,6 +10,7 @@ ## 8.28.0 (2024-07-14) * Fixed a case where recently saved tags or loadouts might not appear if DIM Sync is down, even though they were still saved. +* Dropdown no longer flickers on Firefox. ## 8.27.0 (2024-07-07) diff --git a/src/app/dim-ui/PressTip.tsx b/src/app/dim-ui/PressTip.tsx index 2bf4c71d6d..c8bb4d5331 100644 --- a/src/app/dim-ui/PressTip.tsx +++ b/src/app/dim-ui/PressTip.tsx @@ -98,6 +98,7 @@ function Control({ reference: triggerRef, arrowClassName: styles.arrow, placement, + state: open, }); if (!tooltip) { diff --git a/src/app/dim-ui/Select.tsx b/src/app/dim-ui/Select.tsx index 542f52e761..e178a499de 100644 --- a/src/app/dim-ui/Select.tsx +++ b/src/app/dim-ui/Select.tsx @@ -84,6 +84,7 @@ export default function Select({ reference: buttonRef, placement: 'bottom-start', offset: 2, + state: isOpen, }); if (!selectedItem) { diff --git a/src/app/dim-ui/destiny-symbols/SymbolsPicker.tsx b/src/app/dim-ui/destiny-symbols/SymbolsPicker.tsx index cbd2c0cddb..2892a53f46 100644 --- a/src/app/dim-ui/destiny-symbols/SymbolsPicker.tsx +++ b/src/app/dim-ui/destiny-symbols/SymbolsPicker.tsx @@ -121,6 +121,7 @@ function SymbolsPickerButton({ reference: controlRef, arrowClassName: '', placement: 'top', + state: open, }); // A user should be able to click multiple symbols to insert multiple symbols sequentially, diff --git a/src/app/dim-ui/usePopper.ts b/src/app/dim-ui/usePopper.ts index 22d674de9c..581c7c227b 100644 --- a/src/app/dim-ui/usePopper.ts +++ b/src/app/dim-ui/usePopper.ts @@ -107,6 +107,7 @@ export function usePopper({ offset, fixed, padding, + state, }: { /** A ref to the rendered contents of a popper-positioned item */ contents: React.RefObject; @@ -125,6 +126,13 @@ export function usePopper({ /** Is this placed on a fixed item? Workaround for https://github.com/popperjs/popper-core/issues/1156. TODO: make a "positioning context" context value for this */ fixed?: boolean; padding?: Padding; + + /** + * An optional state variable. Use this to have popper recalculate the position of + * a popover when conditionally rendering the contents of the popover (thus + * causing the dimensions to potentially change). + */ + state?: boolean; }) { const popper = useRef(); @@ -160,5 +168,5 @@ export function usePopper({ } return destroy; - }); + }, [reference, contents, state]); } From a61e79be434f6674b058409340de6dae6b427a50 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Sun, 28 Jul 2024 21:49:56 -0400 Subject: [PATCH 2/6] update changelog --- docs/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index f9495d98da..e688992144 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -1,5 +1,7 @@ ## Next +* Dropdown no longer flickers on Firefox. + ## 8.29.0 (2024-07-21) * Fixed a case where invalid loadouts would be repeatedly rejected by DIM Sync. Now they'll be rejected once and be removed. @@ -10,7 +12,6 @@ ## 8.28.0 (2024-07-14) * Fixed a case where recently saved tags or loadouts might not appear if DIM Sync is down, even though they were still saved. -* Dropdown no longer flickers on Firefox. ## 8.27.0 (2024-07-07) From 22f8ec6a64de438f543476ce08e6590142f7cdea Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Mon, 5 Aug 2024 21:15:52 -0400 Subject: [PATCH 3/6] fix a few issues with popovers not re-rendering --- src/app/dim-ui/Dropdown.tsx | 17 ++-- src/app/dim-ui/PressTip.tsx | 1 - src/app/dim-ui/Select.tsx | 16 ++-- .../dim-ui/destiny-symbols/SymbolsPicker.tsx | 1 - src/app/dim-ui/usePopper.ts | 82 ++++++++++--------- 5 files changed, 63 insertions(+), 54 deletions(-) diff --git a/src/app/dim-ui/Dropdown.tsx b/src/app/dim-ui/Dropdown.tsx index 1eb7dde61e..bfa05e456c 100644 --- a/src/app/dim-ui/Dropdown.tsx +++ b/src/app/dim-ui/Dropdown.tsx @@ -74,13 +74,16 @@ export default function Dropdown({ const buttonRef = useRef(null); const menuRef = useRef(null); - usePopper({ - contents: menuRef, - reference: buttonRef, - placement, - offset, - fixed, - }); + usePopper( + { + contents: menuRef, + reference: buttonRef, + placement, + offset, + fixed, + }, + [isOpen, items], + ); return (
diff --git a/src/app/dim-ui/PressTip.tsx b/src/app/dim-ui/PressTip.tsx index c8bb4d5331..2bf4c71d6d 100644 --- a/src/app/dim-ui/PressTip.tsx +++ b/src/app/dim-ui/PressTip.tsx @@ -98,7 +98,6 @@ function Control({ reference: triggerRef, arrowClassName: styles.arrow, placement, - state: open, }); if (!tooltip) { diff --git a/src/app/dim-ui/Select.tsx b/src/app/dim-ui/Select.tsx index e178a499de..57760ce996 100644 --- a/src/app/dim-ui/Select.tsx +++ b/src/app/dim-ui/Select.tsx @@ -79,13 +79,15 @@ export default function Select({ ); const [dropdownHeight, setDropdownHeight] = useState(); - usePopper({ - contents: menuRef, - reference: buttonRef, - placement: 'bottom-start', - offset: 2, - state: isOpen, - }); + usePopper( + { + contents: menuRef, + reference: buttonRef, + placement: 'bottom-start', + offset: 2, + }, + [isOpen, items], + ); if (!selectedItem) { throw new Error('value must correspond to one of the provided options'); diff --git a/src/app/dim-ui/destiny-symbols/SymbolsPicker.tsx b/src/app/dim-ui/destiny-symbols/SymbolsPicker.tsx index 2892a53f46..cbd2c0cddb 100644 --- a/src/app/dim-ui/destiny-symbols/SymbolsPicker.tsx +++ b/src/app/dim-ui/destiny-symbols/SymbolsPicker.tsx @@ -121,7 +121,6 @@ function SymbolsPickerButton({ reference: controlRef, arrowClassName: '', placement: 'top', - state: open, }); // A user should be able to click multiple symbols to insert multiple symbols sequentially, diff --git a/src/app/dim-ui/usePopper.ts b/src/app/dim-ui/usePopper.ts index 581c7c227b..3b9b04c876 100644 --- a/src/app/dim-ui/usePopper.ts +++ b/src/app/dim-ui/usePopper.ts @@ -97,43 +97,38 @@ const popperOptions = ( }; }; -export function usePopper({ - contents, - reference, - arrowClassName, - menuClassName, - boundarySelector, - placement, - offset, - fixed, - padding, - state, -}: { - /** A ref to the rendered contents of a popper-positioned item */ - contents: React.RefObject; - /** An ref to the item that triggered the popper, which anchors it */ - reference: React.RefObject; - /** A class used to identify the arrow */ - arrowClassName?: string; - /** A class used to identify the sidecar menu */ - menuClassName?: string; - /** An optional additional selector for a "boundary area" */ - boundarySelector?: string; - /** Placement preference of the popper. Defaults to "auto" */ - placement?: Placement; - /** Offset of how far from the element to shift the popper. */ - offset?: number; - /** Is this placed on a fixed item? Workaround for https://github.com/popperjs/popper-core/issues/1156. TODO: make a "positioning context" context value for this */ - fixed?: boolean; - padding?: Padding; - - /** - * An optional state variable. Use this to have popper recalculate the position of - * a popover when conditionally rendering the contents of the popover (thus - * causing the dimensions to potentially change). - */ - state?: boolean; -}) { +export function usePopper( + { + contents, + reference, + arrowClassName, + menuClassName, + boundarySelector, + placement, + offset, + fixed, + padding, + }: { + /** A ref to the rendered contents of a popper-positioned item */ + contents: React.RefObject; + /** An ref to the item that triggered the popper, which anchors it */ + reference: React.RefObject; + /** A class used to identify the arrow */ + arrowClassName?: string; + /** A class used to identify the sidecar menu */ + menuClassName?: string; + /** An optional additional selector for a "boundary area" */ + boundarySelector?: string; + /** Placement preference of the popper. Defaults to "auto" */ + placement?: Placement; + /** Offset of how far from the element to shift the popper. */ + offset?: number; + /** Is this placed on a fixed item? Workaround for https://github.com/popperjs/popper-core/issues/1156. TODO: make a "positioning context" context value for this */ + fixed?: boolean; + padding?: Padding; + }, + deps: React.DependencyList = [], +) { const popper = useRef(); const destroy = () => { @@ -168,5 +163,16 @@ export function usePopper({ } return destroy; - }, [reference, contents, state]); + }, [ + contents, + reference, + arrowClassName, + menuClassName, + boundarySelector, + placement, + offset, + fixed, + padding, + deps, + ]); } From d57b594d47208d92b1cb8ede3e0739cfafb663e5 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Wed, 7 Aug 2024 19:23:26 -0400 Subject: [PATCH 4/6] spread syntax --- src/app/dim-ui/usePopper.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/dim-ui/usePopper.ts b/src/app/dim-ui/usePopper.ts index 3b9b04c876..de69f2a1f8 100644 --- a/src/app/dim-ui/usePopper.ts +++ b/src/app/dim-ui/usePopper.ts @@ -173,6 +173,6 @@ export function usePopper( offset, fixed, padding, - deps, + ...deps, ]); } From 96a23fa078861f8ac7ace92ead30bf454c4367cc Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Wed, 7 Aug 2024 19:30:07 -0400 Subject: [PATCH 5/6] loadout tooltip is positioned correctly --- src/app/dim-ui/PressTip.tsx | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/app/dim-ui/PressTip.tsx b/src/app/dim-ui/PressTip.tsx index 2bf4c71d6d..35a07706bb 100644 --- a/src/app/dim-ui/PressTip.tsx +++ b/src/app/dim-ui/PressTip.tsx @@ -93,12 +93,15 @@ function Control({ const pressTipRoot = useContext(PressTipRoot); const [customization, customizeTooltip] = useState({ className: null }); - usePopper({ - contents: tooltipContents, - reference: triggerRef, - arrowClassName: styles.arrow, - placement, - }); + usePopper( + { + contents: tooltipContents, + reference: triggerRef, + arrowClassName: styles.arrow, + placement, + }, + [open], + ); if (!tooltip) { const { style } = rest; From cf052171d348f1c0183d4bbd60e0a69d1e80d48e Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Wed, 7 Aug 2024 19:35:47 -0400 Subject: [PATCH 6/6] add comment and eslint ignore --- src/app/dim-ui/usePopper.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/app/dim-ui/usePopper.ts b/src/app/dim-ui/usePopper.ts index de69f2a1f8..32944af420 100644 --- a/src/app/dim-ui/usePopper.ts +++ b/src/app/dim-ui/usePopper.ts @@ -173,6 +173,14 @@ export function usePopper( offset, fixed, padding, + + /** + * Doing ...deps allows us to pass dependencies from the components that rely on + * usePopper. Certain popovers are only shown when specific conditions are met, + * so by making those conditions dependencies we can position the popover + * correctly once the popover is actually shown. + */ + // eslint-disable-next-line react-hooks/exhaustive-deps ...deps, ]); }