DOP-3614: Upgrade LG Tables to v12 and lower zebra striping row threshold #1164

rayangler · 2024-07-12T15:49:51Z

Stories/Links:

Current Behavior:

Atlas docs
Main branch staging - For convenience, I compiled some examples of tables onto a single test page based on existing examples spread across the repo.

Staging Links:

Atlas staging - For convenience, I compiled some examples of tables onto a single test page based on existing examples spread across the repo.

Notes:

Sorry for big PR.
Upgrades LG table from v6 to v12.
Adds zebra striping for tables with 4+ body rows ONLY if the table is not nested within another table, and if the table does not have any tables nested within any of its rows.
Refactors ListTable component to accommodate stylistic differences and structural changes. I tried to make sure we avoided keeping any specific CSS that was specific to older versions of the components, while contextualizing the ones we do keep (this was useful for styles/properties that only affected VERY specific content within tables).
Adds new AncestorComponentContext to help track if a component (table) is an ancestor of any component. I tried to keep this generalized so that we can extend it to other components besides tables if needed. I wanted to avoid prop drilling so that we don't accidentally lose track of the prop. Feedback is encouraged on this pattern!
Special thanks to DOP 3614: Upgrading ListTable component, adding striping rows #897 for being a useful reference.

Known issues

Tables nested in admonitions may have awkward styling in dark mode. Keeping this as-is since the style is mostly dependent on the admonition's background color.

README updates

- This PR introduces changes that should be reflected in the README, and I have made those updates.
- This PR does not introduce changes that should be reflected in the README

…take-2

rayangler · 2024-07-12T15:55:27Z

src/components/ListTable.js

+      const isStub = colIndex <= stubColumnCount - 1;
+      const role = isStub ? 'rowheader' : null;
+
      return (
        <Cell


I wanted to conditionally render this as th if it's a stub column cell, but the component was printing out a warning about how the table should use Cell. Unfortunately, doesn't seem like the as prop is supported for this either.

I settled on sticking to Cell to avoid creating unnecessary warnings in tests and builds.

rayangler · 2024-07-12T16:01:47Z

src/components/ListTable.js

-      ? children[0].children[0].children.slice(0, headerRowCount)
-      : [{ children: Array(columnCount).fill({ type: 'text', value: '', children: [] }) }];
+  // Check if :header-rows: 0 is specified or :header-rows: is omitted
+  const headerRows = headerRowCount > 0 ? children[0].children[0].children.slice(0, headerRowCount) : [];


Avoided the spoofing since the component doesn't seem to crash anymore without header rows. The change was mostly to avoid needing to have code that was reliant on an unused version of the LG table. I don't feel strongly about this, so if it seems better to add back in, we can.

The main benefit of not spoofing is that it avoids the need for unstyleThead. The drawback is that we need to keep track of column widths via colgroup (see code below) instead of through CSS on the header's th cells, in the event that some table has set widths but no headers.

im pro <colgroup>! this keeps all columns (unless overwritten by css property) in line. great addition

mayaraman19

WOW! This is amazing, thank you for putting together all the different kinds of tables. They all look fantastic!

I am a little bit confused on the AncestorComponentContext. I think this is a wonderful idea but I think some explanation, perhaps using the example of how it works here in Table, would be helpful for me to internalize better what's going on

rayangler · 2024-07-12T21:02:16Z

I am a little bit confused on the AncestorComponentContext. I think this is a wonderful idea but I think some explanation, perhaps using the example of how it works here in Table, would be helpful for me to internalize better what's going on

@mayaraman19 For sure! Sorry, this might be long, but I honestly didn't know how deep I should go. Please let me know if I can be clearer on anything, if I should leave any comments in particular within the code to clarify certain parts, or if I just yapped a whole lotta nothing. 😆

So right now, the idea is that the context has an object that will be used to track any and all components that it sees. For our use case right now, it's only "table", but this can eventually be extended to any component (or combination of components) if we want to. By default, any component in the context's object that we want to track will be falsy (as denoted in this default value). When the context hook is initially used by any topmost-level ListTable, it will use this default/falsy value (used by the ListTable here). The provider the topmost ListTable renders will take the default/falsy value originally instantiated from the first context, sees that we're passing in table as a value, and then creates a new separate instance of the context with the object with the updated component. This way, when a ListTable is nested anywhere within the topmost ListTable and it calls AncestorComponentContext, the context's object will show table: true now instead of false. The topmost Listable's context will still show table: false though since the original instance of the context is unaffected.

It'll be like:

// table: false; this is the default value of AncestorComponentContext
<ListTable>
  <AncestorComponentContextProvider component={'table'}>

    {/* table: true; saw the "table" from the provider immediately above */}
    <ListTable>
      <AncestorComponentContextProvider component={'table'}>
        {/* table: true; this shouldn't change since "table" was already true */}
        <ListTable>
          ...
        </ListTable>
      </AncestorComponentContextProvider>
    </ListTable>

    <div>
      {/* table: true */}
      <ListTable>
        ...
      </ListTable>
    </div>

  </AncestorComponentContextProvider>
</ListTable>

React has an interesting behavior with context providers such that if the same type of context provider is used in multiple places or is nested within itself, the instance of that context that a component uses will be from the most recent parent/ancestor provider. The AncestorComponentContextProvider works around this by grabbing the object of components/booleans from the context immediately before it (in respect to the React node tree), and uses it as the baseline for the new provider's list/object of ancestors (seen here). (In other words, the latest instance of AncestorComponentContextProvider takes the data from the previous instance and adds new data on top of it, if needed.) This is essentially what helps persist the context data across any number of instances of AncestorComponentContextProvider. This isn't very relevant for our ListTable example because it'll always pass down table to the provider to mark the component as true. However, tracking the previous ancestors is beneficial so that if multiple instances of this provider is used to track more than 1 type of component, both components will be marked as true, regardless of how deep the component using the context is and regardless of what component the last provider is tracking (as seen in this test).

This is somewhat similar to prop drilling in that new instances of some component/provider is passing along the same data from previous instances (except in this case, we're also potentially modifying said data). The main difference here is that we don't need to worry about every single component that can be nested in ListTable (or any parent component) needing to drill this one prop indefinitely. Thinking about it now, I also don't know how performant this solution is. I couldn't really find a good example of this implementation. (EDIT: this article is actually fairly similar but for only one type of component! If we want to adopt this pattern and have different hooks for each type of component, that's fine with me as well!)

seungpark

mostly LGTM! thanks for working on this! minor catches below but overall looks great !

src/components/ListTable.js

tests/context/ancestor-components-context.test.js

seungpark · 2024-07-15T14:00:35Z

tests/unit/ListTable.test.js

    const wrapper = mountListTable(data);
-    expect(wrapper.queryAllByRole('columnheader')).toHaveLength(6);
+    expect(wrapper.queryAllByRole('columnheader')).toHaveLength(0);
  });

  it('displays no content in the header row', () => {


minor nit. i don't think the list table with fixed widths specify the no header row. its the lack of option :header-rows: CMIIW

You are correct. Test title was more relevant back when we were spoofing content for empty header row. I'll change the title.

Updated the test title to be more accurate. Let me know if it looks good to you

…take-2

seungpark

this LGTM ! thanks for clear comments and the staging. can we get a copy of the ReST perhaps in a link ! thank you!

seungpark · 2024-07-15T20:11:04Z

src/components/ListTable.js

-      ? children[0].children[0].children.slice(0, headerRowCount)
-      : [{ children: Array(columnCount).fill({ type: 'text', value: '', children: [] }) }];
+  // Check if :header-rows: 0 is specified or :header-rows: is omitted
+  const headerRows = headerRowCount > 0 ? children[0].children[0].children.slice(0, headerRowCount) : [];


im pro <colgroup>! this keeps all columns (unless overwritten by css property) in line. great addition

rayangler · 2024-07-15T22:09:49Z

@seungpark Here's a link to the rST

rayangler added 23 commits July 9, 2024 17:51

Get header cells working

b2f3cb9

Set up basic cell styling

dd948da

Set up cell styling

b2b8bd4

Set up cell styling

b93a98a

Set up cell styling

47b496d

Don't include headerRows if no headerRows

5c45fe7

Adjust widths without table headers

69006e9

Adjust widths without table headers

ce29eb3

Fix spacing in paragraphs and admonitions

d2d4f27

Table element style

2f44a2e

Support footnote refs again

4a4ee70

Figure out stub cells

f9ffc21

Finally work on zebra striping; dark mode

4f8c738

Clean up

d8ee74d

Fix tests

e1fffe7

Fix key and propType errors

ef92558

Remove th element for stub column

d1a723b

Update snapshots

f24ff64

Format

6d31560

Merge branch 'main' of https://github.com/mongodb/snooty into DOP-3614

02c2857

Inherit bg color for thead in case the table is nested

7fa7318

Identify if table is nested or has nested tables

4d64338

Merge branch 'main' of https://github.com/mongodb/snooty into DOP-3614-…

c7b5d72

…take-2

rayangler commented Jul 12, 2024

View reviewed changes

rayangler added 2 commits July 12, 2024 12:11

Undo bg inheritance since table not nested anymore

fdf14e7

Remove accidental th

bd2fcad

rayangler marked this pull request as ready for review July 12, 2024 16:17

rayangler requested review from seungpark, mmeigs and mayaraman19 July 12, 2024 16:19

mayaraman19 reviewed Jul 12, 2024

View reviewed changes

seungpark reviewed Jul 15, 2024

View reviewed changes

rayangler added 3 commits July 15, 2024 15:31

Merge branch 'main' of https://github.com/mongodb/snooty into DOP-3614-…

f3936c4

…take-2

Address PR comments

70a4837

Update snapshot

6a71919

seungpark approved these changes Jul 15, 2024

View reviewed changes

rayangler merged commit 9c3e04d into main Jul 15, 2024
4 checks passed

rayangler deleted the DOP-3614-take-2 branch July 15, 2024 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOP-3614: Upgrade LG Tables to v12 and lower zebra striping row threshold #1164

DOP-3614: Upgrade LG Tables to v12 and lower zebra striping row threshold #1164

rayangler commented Jul 12, 2024 •

edited

Loading

rayangler Jul 12, 2024 •

edited

Loading

rayangler Jul 12, 2024 •

edited

Loading

seungpark Jul 15, 2024

mayaraman19 left a comment

rayangler commented Jul 12, 2024 •

edited

Loading

seungpark left a comment

seungpark Jul 15, 2024

rayangler Jul 15, 2024 •

edited

Loading

rayangler Jul 15, 2024

seungpark left a comment

seungpark Jul 15, 2024

rayangler commented Jul 15, 2024

DOP-3614: Upgrade LG Tables to v12 and lower zebra striping row threshold #1164

DOP-3614: Upgrade LG Tables to v12 and lower zebra striping row threshold #1164

Conversation

rayangler commented Jul 12, 2024 • edited Loading

Stories/Links:

Current Behavior:

Staging Links:

Notes:

README updates

rayangler Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

rayangler Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

seungpark Jul 15, 2024

Choose a reason for hiding this comment

mayaraman19 left a comment

Choose a reason for hiding this comment

rayangler commented Jul 12, 2024 • edited Loading

seungpark left a comment

Choose a reason for hiding this comment

seungpark Jul 15, 2024

Choose a reason for hiding this comment

rayangler Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

rayangler Jul 15, 2024

Choose a reason for hiding this comment

seungpark left a comment

Choose a reason for hiding this comment

seungpark Jul 15, 2024

Choose a reason for hiding this comment

rayangler commented Jul 15, 2024

rayangler commented Jul 12, 2024 •

edited

Loading

rayangler Jul 12, 2024 •

edited

Loading

rayangler Jul 12, 2024 •

edited

Loading

rayangler commented Jul 12, 2024 •

edited

Loading

rayangler Jul 15, 2024 •

edited

Loading