Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not use spectrum for colorpicker #1335

Merged
merged 1 commit into from
May 15, 2022
Merged

Do not use spectrum for colorpicker #1335

merged 1 commit into from
May 15, 2022

Conversation

jtojnar
Copy link
Member

@jtojnar jtojnar commented May 14, 2022

It is unmaintained, depends on jQuery and Parcel no longer wants to parse its CSS (asterisk hack before display property for IE 7 support).It is unmaintained, depends on jQuery and Parcel no longer wants to parse its CSS (asterisk hack before display property for IE 7 support).

Fixes: #1268

Unfortunately, this probably will not work due to the sidebar containing the menu button being position: fixed and overflow: hidden.

The popup is cut of by overflow

We might need to use something like popper to get reasonable placement of the popup.

Switched to Floating UI (Popper successor) and it works okay.

It would be nice to have arrow pointing to the tag being changed but I did not manage to align it properly.

Arrow patch
diff --git a/assets/js/templates/ColorChooser.jsx b/assets/js/templates/ColorChooser.jsx
index 084440b9..d2c63dde 100644
--- a/assets/js/templates/ColorChooser.jsx
+++ b/assets/js/templates/ColorChooser.jsx
@@ -1,5 +1,5 @@
-import React, { useCallback, useContext, useMemo } from 'react';
-import { useFloating, autoUpdate, flip, offset, shift } from '@floating-ui/react-dom';
+import React, { useCallback, useContext, useMemo, useRef } from 'react';
+import { useFloating, arrow, autoUpdate, flip, offset, shift } from '@floating-ui/react-dom';
 import PropTypes from 'prop-types';
 import { Button as MenuButton, Wrapper as MenuWrapper, Menu, MenuItem } from 'react-aria-menubutton';
 import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
@@ -54,12 +54,16 @@ export default function ColorChooser({tag, onChange}) {
         },
     );
 
+    const arrowRef = useRef();
+
     const {
         x: menuX,
         y: menuY,
         reference: buttonRef,
         floating: floatingRef,
         strategy: positionStrategy,
+        placement: menuPlacement,
+        middlewareData: {arrow: {x: arrowX, y: arrowY} = {}},
     } = useFloating({
         placement: 'right-start',
         strategy: 'fixed',
@@ -67,12 +71,26 @@ export default function ColorChooser({tag, onChange}) {
             offset({ mainAxis: 16 }),
             shift(),
             flip(),
+            arrow({
+                element: arrowRef,
+                // Prevent overflowing border radius
+                // padding: 8,
+                //
+                // centerOffset: 11.31,
+            }),
         ],
         whileElementsMounted: autoUpdate,
     });
 
     const _ = useContext(LocalizationContext);
 
+    const staticSide = {
+        top: 'bottom',
+        right: 'left',
+        bottom: 'top',
+        left: 'right',
+    }[menuPlacement.split('-')[0]];
+
     return (
         <MenuWrapper
             className="popup-menu-wrapper color"
@@ -98,6 +116,18 @@ export default function ColorChooser({tag, onChange}) {
                     left: menuX ?? '',
                 }}
             >
+                <div
+                    className="arrow"
+                    ref={arrowRef}
+                    style={{
+                        top: arrowY ?? '',
+                        left: arrowX ?? '',
+                        right: '',
+                        bottom: '',
+                        // Needs to match “-$arrow-dimension / 2” from popup-menu.scss
+                        [staticSide]: '-0.707em',
+                    }}
+                />
                 {palette.map((color) => (
                     <ColorButton
                         key={color}
diff --git a/assets/styles/popup-menu.scss b/assets/styles/popup-menu.scss
index df3053c7..b3679898 100644
--- a/assets/styles/popup-menu.scss
+++ b/assets/styles/popup-menu.scss
@@ -1,3 +1,5 @@
+@use 'sass:math';
+
 .popup-menu-wrapper {
     display: inline-block;
     position: relative;
@@ -25,3 +27,46 @@
     background-color: var(--primary);
     color: var(--primary-contrast);
 }
+
+.arrow,
+.arrow::before {
+    position: absolute;
+    background: inherit;
+    z-index: -1;
+    // background: rebeccapurple;
+}
+
+$arrow-side: 1rem;
+$arrow-dimension: math.hypot($arrow-side, $arrow-side);
+
+.arrow {
+    box-sizing: border-box;
+    width: $arrow-dimension;
+    height: $arrow-dimension;
+    padding: math.div($arrow-dimension - $arrow-side, 2);
+    visibility: hidden;
+}
+
+.arrow::before {
+    width: $arrow-side;
+    height: $arrow-side;
+    visibility: visible;
+    content: '';
+    transform: rotate(45deg);
+}
+
+.popup-menu[data-popper-placement^='top'] > .arrow {
+    bottom: -0.5em;
+}
+
+.popup-menu[data-popper-placement^='bottom'] > .arrow {
+    top: -0.5em;
+}
+
+.popup-menu[data-popper-placement^='left'] > .arrow {
+    right: -0.5em;
+}
+
+.popup-menu[data-popper-placement^='right'] > .arrow {
+    left: -0.5em;
+}

We should probably also freeze scrolling why the popup is open but that is not anything new.

@jtojnar jtojnar marked this pull request as draft May 14, 2022 20:05
@jtojnar jtojnar marked this pull request as ready for review May 15, 2022 00:50
@jtojnar jtojnar added this to the 2.19 milestone May 15, 2022
@jtojnar jtojnar force-pushed the colorpicker branch 2 times, most recently from 0c566d9 to ef016b0 Compare May 15, 2022 01:12
It is unmaintained, depends on jQuery and Parcel no longer wants to parse its CSS (asterisk hack before display property for IE 7 support).

We need to use popper/floating-ui to correctly position the popup since the sidebar it is in has `position: fixed` and `overflow: hidden`.
@jtojnar jtojnar merged commit 4ff96f3 into master May 15, 2022
@jtojnar jtojnar deleted the colorpicker branch May 15, 2022 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace spectrum with react-based color picker
1 participant