S2 table #6778

devongovett · 2024-07-26T23:13:51Z

See followups that still need to be implemented here: RSP Component Milestones (view)

row focus ring needs row render function first

this will need changes to RAC table as well....

…utton focus visible also tested moving the sort descriptor in front of the column text andwithout reserving room

…s ring

…r drag/checkbox columns

…sizers are skipped

… values

…le body outline

rspbot · 2024-09-17T20:34:27Z

Build successful! 🎉

devongovett · 2024-09-17T17:09:22Z

packages/@react-spectrum/s2/src/Table.tsx

+}
+
+// TODO: Note that loadMore and loadingState are now on the Table instead of on the TableBody
+export interface TableProps extends Omit<RACTableProps, 'style' | 'disabledBehavior' | 'className' | 'onRowAction' | 'selectionBehavior' | 'onScroll' | 'onCellAction' | 'dragAndDropHooks'>, StyleProps, S2TableProps {


Since table allows overriding the height, we should extends UnsafeStyles here and add styles using StylesPropWithHeight.

good call, I'm not sure why it didn't complain in story where I make use of both height and width via styles hmm

devongovett · 2024-09-17T17:19:17Z

packages/@react-spectrum/s2/src/Table.tsx

+        <InternalTableContext.Provider value={context}>
+          <RACTable
+            ref={scrollRef}
+            style={{WebkitTransform: 'translateZ(0)'}}


only webkit? What is this fixing?

yep, webkit only, this fixes the issue of checkboxes appearing above the scrollbar, I'll add a comment

devongovett · 2024-09-17T17:21:35Z

packages/@react-spectrum/s2/src/Table.tsx

+  let loadMoreSpinner = (
+    <UNSTABLE_TableLoadingIndicator className={style({height: 'full', width: 'full'})}>
+      <div className={centeredWrapper}>
+        <ProgressCircle


well we should probably ask spectrum. I still think smaller is better, or maybe it should depend on the density.

devongovett · 2024-09-17T17:25:23Z

packages/@react-spectrum/s2/src/Table.tsx

+        <Collection items={items}>
+          {children}
+        </Collection>
+        {loadingState === 'loadingMore' && [...items].length > 0 && loadMoreSpinner}


Why the length check here but not below? I'd be a little concerned about performance of the spread as well so if we can avoid it that would be good.

We should probably also discuss how table should support ghost loading at some point, and what the default should be.

the length check here is to prevent a situation where somehow the `loadingState = "loadingMore"' but no items are actually loaded aka the below:

For loadingState = "loading" we don't need these checks because the rendering of that circle/empty state is provided to renderEmptyState instead so RAC table handles rendering it only when the collection size is 0.

We could consider the "loadingMore" case a user error and get rid of the length check perhaps, what do you think? Otherwise I need some way of knowing the collection is empty...

Hmm yeah I'd consider that programmer error I think

sounds good, removed the check

devongovett · 2024-09-17T17:29:13Z

packages/@react-spectrum/s2/src/Table.tsx

+
+function CellFocusRing(props: {isFocusVisible: boolean}) {
+  let {isFocusVisible} = props;
+  return <div role="presentation" style={{position: 'absolute', top: 0, left: 0, bottom: 0, right: 0}} className={style({...cellFocus})({isFocusVisible})} />;


Suggested change

return <div role="presentation" style={{position: 'absolute', top: 0, left: 0, bottom: 0, right: 0}} className={style({...cellFocus})({isFocusVisible})} />;

return <div role="presentation" style={{position: 'absolute', inset: 0}} className={style({...cellFocus})({isFocusVisible})} />;

Or actually, why are the position properties inline instead of in the style macro?

ah it previously was but the cellFocus gets reused in other places so I put this inline to ensure it didn't propagate elsewhere unexpectedly. I dunno why I didn't put it in the style macro in the className, whoops will do thanks!

devongovett · 2024-09-17T20:54:15Z

packages/@react-spectrum/s2/src/Table.tsx

+  insetEnd: size(-6),
+  cursor: {
+    default: 'none',
+    resizableDirection: {


Eventually we need our custom cursors here like we had in S1?

oh thats right, completely slipped my mind, missed it since I didn't tag it with TODO lol. I'll add it to the followup ticket and see if we need to submit something to the icons team

filed an airtable ticket for those cursors

devongovett · 2024-09-17T20:56:18Z

packages/@react-spectrum/s2/src/Table.tsx

+        // TODO: Need to wait for the menu to fully transition out, otherwise mouse movements may hover the menu items and steal focus from the resizer,
+        // causing resizing to end prematurely due to blur. Open to ideas for how to handle this since it affects RAC. Ideally we'd freeze
+        // focus from being able to move if we are in resizing mode except if the user clicks away or moves focus with something other than hover
+        setTimeout(() => startResize(), 200);


We could mark popovers/overlays as inert when they are transitioning out maybe

devongovett · 2024-09-17T21:30:35Z

packages/@react-spectrum/s2/src/utils.ts

+import {useMediaQuery} from '@react-spectrum/utils';
+
+export function useIsMobileDevice(): boolean {
+  return useMediaQuery('(max-width: 640px)');


Since this is being used to determine the scale, shouldn't it look for @media not ((hover: hover) and (pointer: fine)), which is what the style macro uses for that?

I considered doing that but I figured we'd want to follow a similar logic style that we had in S1 in preparation for Trays aka we would check screen size. If we use the touch media query that the style macro uses, that would match large screen touch devices right?

But this is being used to determine pixel sizes not whether to show a tray right? The style macro uses the media query I posted to do the same thing, ie scale up dimensions by 25%. Whether to show a tray is indeed based on screen size but that's different from touch scale, which might also occur on large screen devices like iPads or laptops.

ahh right, makes sense, guess I conflated the two. I'll make a separate useScale that uses the media query and use it for table

… doesnt get canceled if the user clicks the resize option and hovers another item while the popover is transitioning out it can cause resizing to be canceled. This prevents that case from happening

rspbot · 2024-09-18T00:13:21Z

Build successful! 🎉

… check warning is a compat one for React 19, items check is uneeded due to user error

rspbot · 2024-09-18T20:52:50Z

Build successful! 🎉

rspbot · 2024-09-19T18:54:39Z

Build successful! 🎉

snowystinger · 2024-09-20T05:25:48Z

packages/react-aria-components/test/DatePicker.test.js

    user = userEvent.setup({delay: null, pointerMap});
  });
  it('provides slots', async () => {
+    // Mock console.error for React Canary "Received the string `true` for the boolean attribute `inert`." warning


So these errors are going to appear in downstream projects tests as well

snowystinger · 2024-09-20T05:25:54Z

packages/react-aria-components/src/Popover.tsx

        slot={props.slot || undefined}
        style={style}
+        // @ts-ignore - React < 19 compat
+        inert={(isEntering || isExiting) ? 'true' : undefined}


what is actually the issue?

I think this is causing focus not to move into the popover when it opens now. e.g. try opening the column header menu with a keyboard - focus stays on the header instead of going to the menu item.

Reverted the inert change and used pointer-events: none instead so it only applies to mouse during the exit animation.

# Conflicts: # packages/@react-spectrum/s2/stories/Table.stories.tsx # packages/react-aria-components/test/ComboBox.test.js # packages/react-aria-components/test/Select.test.js

…resizing doesnt get canceled" This reverts commit 9cfb896.

This reverts commit 7006a76. # Conflicts: # packages/react-aria-components/test/ComboBox.test.js # packages/react-aria-components/test/Select.test.js

…nert

It has a bunch of internal stuff on it. We don't need the exact height for the resizer, we just need a large enough value that it goes past the end of the table. 100vh should work.

packages/@react-spectrum/s2/style/spectrum-theme.ts

packages/@react-spectrum/s2/package.json

rspbot · 2024-09-25T18:18:45Z

Build successful! 🎉

devongovett · 2024-09-25T18:19:33Z

I approve! ✅

(GitHub won't let me since I opened the PR 😂)

devongovett and others added 30 commits July 26, 2024 15:13

Add Spectrum 2 docs to storybook

0d72ba1

progress on table styles

928ba4b

fix highlight checkbox rendering and add rounded corners to body

a43a2b0

fix checkbox styles, update to columns, font family

3ff0b80

refactor cell focus ring and add row focus ring

e62fdd4

row focus ring needs row render function first

wrapping up disabled row styles

286599b

empty/loading state brainstorming

0be7cb0

Replace empty state with loading spinner

2097e14

add basic sorting

378f78d

attempt to add showDivider

902160e

this will need changes to RAC table as well....

Fix select all checkbox click

68f0f2f

add placeholder for DnD and use new icon for sortable column

5edf997

update HCM colors

54c6c4a

fix DnD story, removed need for sortDescriptor on context, fix drag b…

289a68d

…utton focus visible also tested moving the sort descriptor in front of the column text andwithout reserving room

clean up

fef744b

switch to box shadow on row for grey row underline and selection focu…

ac241bc

…s ring

update colors/sizes from table specific tokens

f6d83fa

updating lightdark so it works with opacity values

e4b1993

add row visible focus style and fix empty columnheader focus style fo…

f7cb581

…r drag/checkbox columns

initial setup for resizing support

febb547

progress on resizer handle styling and menu icons

5b84fd0

fix more resize bugs and some clean up

36cbbda

nubbin and more DnD styling

8dd6ddf

fix drag indicator so it respects density

7c39d02

got verdaccio working, updating row background style and confirmed re…

f7a9b00

…sizers are skipped

updating loading more to use rowLoader

68bcd6c

use isTableEmpty renderProps from local RAC

0dff41c

Add virtualizer and begin fixing styles

d4f5fb7

add scale, update to column widths, some updates to reflect new token…

7cf41ff

… values

add async loading stories and updates to use background image for tab…

8200816

…le body outline

devongovett commented Sep 17, 2024

View reviewed changes

LFDanLu added 2 commits September 17, 2024 16:29

address initial review comments

a4ff0d7

make popovers inert when they are transitioning out so table resizing…

9cfb896

… doesnt get canceled if the user clicks the resize option and hovers another item while the popover is transitioning out it can cause resizing to be canceled. This prevents that case from happening

LFDanLu added 2 commits September 18, 2024 10:09

add useScale and use it in table instead

0e3c7ec

ignore warnings in test for invalid attribute and remove items length…

7006a76

… check warning is a compat one for React 19, items check is uneeded due to user error

LFDanLu added the release label Sep 18, 2024

forgot the forward ref and add descriptions

54119e1

snowystinger reviewed Sep 20, 2024

View reviewed changes

devongovett added 5 commits September 24, 2024 18:10

Merge branch 'main' of github.com:adobe/react-spectrum into s2-table

8ceec14

# Conflicts: # packages/@react-spectrum/s2/stories/Table.stories.tsx # packages/react-aria-components/test/ComboBox.test.js # packages/react-aria-components/test/Select.test.js

Revert "make popovers inert when they are transitioning out so table …

e91dd72

…resizing doesnt get canceled" This reverts commit 9cfb896.

Revert "ignore warnings in test for invalid attribute"

0b5f3fa

This reverts commit 7006a76. # Conflicts: # packages/react-aria-components/test/ComboBox.test.js # packages/react-aria-components/test/Select.test.js

Use pointerEvents: none in popover during exit animation instead of i…

bdf683f

…nert

Don't expose ResizableTableContainerContext publicly

caccd11

It has a bunch of internal stuff on it. We don't need the exact height for the resizer, we just need a large enough value that it goes past the end of the table. 100vh should work.

devongovett commented Sep 25, 2024

View reviewed changes

packages/@react-spectrum/s2/style/spectrum-theme.ts Outdated Show resolved Hide resolved

devongovett commented Sep 25, 2024

View reviewed changes

packages/@react-spectrum/s2/package.json Outdated Show resolved Hide resolved

devongovett added 3 commits September 25, 2024 11:04

Use VisuallyHidden from react-aria monopackage

4a4237e

Revert border style longhands

0c42a65

lint

8307b94

reidbarber approved these changes Sep 25, 2024

View reviewed changes

dannify approved these changes Sep 25, 2024

View reviewed changes

devongovett added this pull request to the merge queue Sep 25, 2024

Merged via the queue into main with commit 5bb6a22 Sep 25, 2024
3 checks passed

devongovett deleted the s2-table branch September 25, 2024 19:17

	return <div role="presentation" style={{position: 'absolute', top: 0, left: 0, bottom: 0, right: 0}} className={style({...cellFocus})({isFocusVisible})} />;
	return <div role="presentation" style={{position: 'absolute', inset: 0}} className={style({...cellFocus})({isFocusVisible})} />;

S2 table #6778

S2 table #6778

Uh oh!

Conversation

devongovett commented Jul 26, 2024 • edited by LFDanLu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rspbot commented Sep 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LFDanLu Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rspbot commented Sep 18, 2024

Uh oh!

rspbot commented Sep 18, 2024

Uh oh!

rspbot commented Sep 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rspbot commented Sep 25, 2024

Uh oh!

devongovett commented Sep 25, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

devongovett commented Jul 26, 2024 •

edited by LFDanLu

Loading

LFDanLu Sep 17, 2024 •

edited

Loading