Table: Add ability to specify sticky columns #1395

ayeshakmaz · 2021-02-23T06:38:52Z

Adds ability to specify sticky column(s)

Test Plan

Added some snapshot tests

netlify · 2021-02-23T06:44:20Z

Deploy preview for gestalt ready!

Built with commit 8d9143c

https://deploy-preview-1395--gestalt.netlify.app

packages/gestalt/src/TableContextProvider.js

packages/gestalt/src/TableHeaderCell.js

AlbertCarreras · 2021-02-25T20:41:58Z

Looking good!

dangerismycat

This looks awesome!! The UI is 100% there. 🙂

dangerismycat · 2021-04-01T18:08:11Z

docs/src/Table.doc.js

+    name="Example: Sticky Column"
+    description="Try scrolling horizontally to see the first column remain in place."
+    defaultCode={`
+<Box width={"50%"} overflow="auto">


nit: use string props directly, no need for the curlies (here and throughout)

dangerismycat · 2021-04-01T18:09:09Z

docs/src/Table.doc.js

+    description="Try scrolling horizontally to see the first column remain in place."
+    defaultCode={`
+<Box width={"50%"} overflow="auto">
+<Table maxHeight={200} stickyColumns={1}>


These code examples are pretty hefty; can you break things up with some whitespace for readability? Since they start collapsed, I'm not really concerned with taking up more real estate

dangerismycat · 2021-04-01T18:09:40Z

docs/src/Table.doc.js

+card(
+  <Example
+    id="stickyColumn3"
+    name="Example: Sticky header and sticky columns"


This UI looks so good!! Nice work!

dangerismycat · 2021-04-01T18:10:57Z

docs/src/Table.doc.js

+                    image={
+                      <Avatar
+                        name="tony avatar"
+                        src="https://i.ibb.co/8948ym5/avenge.png"


These examples are great, but are there copyright issues with using these images? Or is this considered fair use?

That be a good PR stunt for our gestalt open source library! :)

dangerismycat · 2021-04-01T18:13:02Z

packages/gestalt/src/Table.js

 |};

 export default function Table(props: Props): Node {
-  const { borderStyle, children, maxHeight } = props;
+  const { borderStyle, children, maxHeight, stickyColumns } = props;
+  const [showShadowScroll, setShowShadowScroll] = useState(null);


I don't think Flow will be able to properly infer the type here, so you may want to add it. Looks like it's an optional enum? ?('left' | 'right')

nit, I think ryan was suggesting here useState<('left' | 'right'| null)>(null)

dangerismycat · 2021-04-01T18:23:57Z

packages/gestalt/src/TableRow.js

+
+  const renderCellsWithIndex = () => {
+    const cells = [];
+    const tableRowChildrenArray = Children.toArray(props.children);


This can be simplified with Children.map:

const renderCellsWithIndex = () => Children.map(props.children, (item) => { ... return cloneElement(...); });

Another idea to prevent needing this separate faux-render method would be to extract the map function to a helper, so the component return is something like

return <tr ref={rowRef}>{stickyColumns > 0 ? Children.map(props.children, renderCellsWithIndex) : props.children}</tr>;

dangerismycat · 2021-04-01T18:26:44Z

packages/gestalt/src/TableRowExpandable.js

@@ -35,6 +44,9 @@ export default function TableRowExpandable(props: Props): Node {
  const [expanded, setExpanded] = useState(false);
  const hoverStyle = props.hoverStyle || 'gray';
  const cs = hoverStyle === 'gray' ? cx(styles.hoverShadeGray) : null;
+  const { stickyColumns } = useContext(TableContext);
+  const rowRef = React.useRef();


same nit here about importing directly

dangerismycat · 2021-04-01T18:27:42Z

packages/gestalt/src/TableRowExpandable.js

@@ -35,6 +44,9 @@ export default function TableRowExpandable(props: Props): Node {
  const [expanded, setExpanded] = useState(false);
  const hoverStyle = props.hoverStyle || 'gray';
  const cs = hoverStyle === 'gray' ? cx(styles.hoverShadeGray) : null;
+  const { stickyColumns } = useContext(TableContext);
+  const rowRef = React.useRef();
+  const [columnWidths, setColumnWidths] = useState([]);

  const handleButtonClick = ({ event }) => {
    setExpanded(!expanded);


This always makes me nervous…we should use functional updates for this sort of toggling to prevent using stale values: https://reactjs.org/docs/hooks-reference.html#functional-updates

Thanks for pointing to this... I feel we do that across many cmps.

Since it's unrelated to these changes, I'll make a separate ticket to look at this across our cmps

dangerismycat · 2021-04-01T18:28:13Z

packages/gestalt/src/TableRowExpandable.js

+    if (rowRef && rowRef.current && stickyColumns) {
+      const colWidths = [];
+      const tableRowChildrenArray = [...rowRef.current.children];
+      tableRowChildrenArray.forEach((child) => {


Same feedback here re: .map

I wonder if some of this repeated logic could be extracted out into a helper file to share and make it dryier.

dangerismycat · 2021-04-01T18:28:30Z

packages/gestalt/src/TableRowExpandable.js

+    const cells = [];
+    const tableRowChildrenArray = Children.toArray(props.children);
+
+    tableRowChildrenArray.forEach((child, index) => {


same feedback here re: Children.map

docs/src/Table.doc.js

AlbertCarreras · 2021-04-01T20:51:28Z

I really value your work on this shadow... but I must admit it doesn't feel good to me. I feel like a thicker borderline could bring me more peace. haha

AlbertCarreras · 2021-04-01T21:15:11Z

packages/gestalt/src/Table.js

+    };
+  }, [updateShadows]);
+
+  useEffect(() => {


do we need this useEffect?

can we merge wth previous?

AlbertCarreras · 2021-04-01T22:15:36Z

packages/gestalt/src/Table.js

  return (
    <Box
      overflow="auto"
      {...(borderStyle === 'sm' ? { borderStyle: 'sm', rounding: 1 } : {})}
      maxHeight={maxHeight}
+      ref={contentRef}


very stupid nit,,, what about tableRef?

AlbertCarreras · 2021-04-01T22:22:28Z

packages/gestalt/src/TableRow.js


 type Props = {|
  children: Node,
 |};

 export default function TableRow(props: Props): Node {
-  return <tr>{props.children}</tr>;
+  const { stickyColumns } = useContext(TableContext);


import { useTableContext } from './TableContext.js';
then

const { stickyColumns } = useTableContext();

AlbertCarreras · 2021-04-01T22:23:18Z

packages/gestalt/src/TableRow.js

+  const [columnWidths, setColumnWidths] = useState([]);
+
+  useEffect(() => {
+    if (rowRef && rowRef.current && stickyColumns) {


rowRef && rowRef.current >> rowRef?.current

AlbertCarreras · 2021-04-01T22:28:22Z

packages/gestalt/src/TableRow.js

+      });
+      setColumnWidths(colWidths);
+    }
+  }, [rowRef, stickyColumns]);


is needed rowRef? rowRef.current aint needed. I'm not sure about this, just asking a question to discuss and learn.

It isn't actually, this was left over from a previous attempt, good catch!

facebook/react#14387 (comment)

AlbertCarreras · 2021-04-01T22:30:52Z

Looking good. Some Flow issues here and there!

packages/gestalt/src/contexts/TableContext.js

AlbertCarreras

Awesome job! Sticky power!

AlbertCarreras reviewed Feb 25, 2021

View reviewed changes

packages/gestalt/src/TableContextProvider.js Outdated Show resolved Hide resolved

AlbertCarreras reviewed Feb 25, 2021

View reviewed changes

packages/gestalt/src/TableHeaderCell.js Outdated Show resolved Hide resolved

ayeshakmaz force-pushed the ayesha/table-col-locking branch from ce9482d to 5c23992 Compare March 15, 2021 23:44

dangerismycat self-assigned this Mar 23, 2021

ayeshakmaz added 9 commits March 31, 2021 16:34

Table: Add ability to freeze columns

dbdcc7f

Working

e15c655

RTL

27808b1

WIP - white line

017cf54

update

5c4b15c

wip

e2f5bc1

wip

3f78e51

wip

781ac10

sticky columns with shadow

6fcc898

ayeshakmaz force-pushed the ayesha/table-col-locking branch from 698be6d to 6fcc898 Compare March 31, 2021 23:36

ayeshakmaz marked this pull request as ready for review March 31, 2021 23:40

ayeshakmaz requested a review from a team as a code owner March 31, 2021 23:40

Test fixes

b200636

ayeshakmaz changed the title ~~[Prototyping] Table: Add ability to freeze columns~~ Table: Add ability to specify sticky columns Mar 31, 2021

Tests

0a2e815

dangerismycat reviewed Apr 1, 2021

View reviewed changes

AlbertCarreras reviewed Apr 1, 2021

View reviewed changes

docs/src/Table.doc.js Show resolved Hide resolved

AlbertCarreras reviewed Apr 1, 2021

View reviewed changes

ayeshakmaz added 3 commits April 5, 2021 15:15

PR feedback

602997c

Update types

d8a613c

type fixes

2c0ddde

AlbertCarreras reviewed Apr 6, 2021

View reviewed changes

packages/gestalt/src/contexts/TableContext.js Show resolved Hide resolved

fix types

4ea62ea

ayeshakmaz added the minor release Minor release label Apr 6, 2021

ayeshakmaz added 3 commits April 6, 2021 16:13

docs fix

a4bd5a3

state

dca34f3

fix

61baf75

AlbertCarreras self-requested a review April 6, 2021 23:37

AlbertCarreras approved these changes Apr 6, 2021

View reviewed changes

docs bug

8d9143c

ayeshakmaz merged commit 0b7eb05 into pinterest:master Apr 7, 2021

ayeshakmaz deleted the ayesha/table-col-locking branch April 7, 2021 00:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Table: Add ability to specify sticky columns #1395

Table: Add ability to specify sticky columns #1395

ayeshakmaz commented Feb 23, 2021 •

edited

Loading

netlify bot commented Feb 23, 2021 •

edited

Loading

AlbertCarreras commented Feb 25, 2021

dangerismycat left a comment

dangerismycat Apr 1, 2021

dangerismycat Apr 1, 2021

dangerismycat Apr 1, 2021

dangerismycat Apr 1, 2021

AlbertCarreras Apr 6, 2021

dangerismycat Apr 1, 2021

AlbertCarreras Apr 1, 2021

AlbertCarreras Apr 6, 2021 •

edited

Loading

dangerismycat Apr 1, 2021

dangerismycat Apr 1, 2021

dangerismycat Apr 1, 2021

AlbertCarreras Apr 1, 2021

ayeshakmaz Apr 5, 2021

dangerismycat Apr 1, 2021

AlbertCarreras Apr 1, 2021 •

edited

Loading

dangerismycat Apr 1, 2021

AlbertCarreras commented Apr 1, 2021 •

edited

Loading

AlbertCarreras Apr 1, 2021

AlbertCarreras Apr 1, 2021

AlbertCarreras Apr 1, 2021

AlbertCarreras Apr 1, 2021 •

edited

Loading

AlbertCarreras Apr 1, 2021

ayeshakmaz Apr 5, 2021

AlbertCarreras Apr 6, 2021

AlbertCarreras commented Apr 1, 2021

AlbertCarreras left a comment

Table: Add ability to specify sticky columns #1395

Table: Add ability to specify sticky columns #1395

Conversation

ayeshakmaz commented Feb 23, 2021 • edited Loading

Test Plan

netlify bot commented Feb 23, 2021 • edited Loading

AlbertCarreras commented Feb 25, 2021

dangerismycat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlbertCarreras Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlbertCarreras Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlbertCarreras commented Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlbertCarreras Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlbertCarreras commented Apr 1, 2021

AlbertCarreras left a comment

Choose a reason for hiding this comment

ayeshakmaz commented Feb 23, 2021 •

edited

Loading

netlify bot commented Feb 23, 2021 •

edited

Loading

AlbertCarreras Apr 6, 2021 •

edited

Loading

AlbertCarreras Apr 1, 2021 •

edited

Loading

AlbertCarreras commented Apr 1, 2021 •

edited

Loading

AlbertCarreras Apr 1, 2021 •

edited

Loading