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

table: frozen columns #1720

Draft
wants to merge 16 commits into
base: develop
Choose a base branch
from
Draft

table: frozen columns #1720

wants to merge 16 commits into from

Conversation

ChrisLaneAU
Copy link
Contributor

@ChrisLaneAU ChrisLaneAU commented Jul 19, 2024

Adds support for frozen columns in tables. Also fixes the caption so that it doesn't scroll horizontally as the table is scrolled.

View preview

Scroll far left:

image

Scroll middle:

image

Scroll far right:

image

Small screens (below medium breakpoint using container queries):

image

Checklist

Preflight

  • Prefix the PR title with the slug of the package or component - e.g. accordion: Updated padding or docs: Updated header links
  • Describe the changes clearly in the PR description
  • Read and check your code before tagging someone for review
  • Create a changeset file by running yarn changeset. Learn more about change management.

Testing

  • Manually test component in various modern browsers at various sizes (use Browserstack)
  • Manually test component in various devices (phone, tablet, desktop)
  • Manually test component using a keyboard
  • Manually test component using a screen reader
  • Manually tested in dark mode
  • Component meets Web Content Accessibility Guidelines (WCAG) 2.1 standards
  • Add any necessary unit tests (HTML validation, snapshots etc)
  • Run yarn test to ensure tests are passing. If required, run yarn test -u to update any generated snapshots.

Documentation

  • Create or update documentation on the website
  • Create or update stories for Storybook
  • Create or update stories for Playroom snippets

@ChrisLaneAU ChrisLaneAU self-assigned this Jul 19, 2024
Copy link

changeset-bot bot commented Jul 19, 2024

🦋 Changeset detected

Latest commit: 5d533df

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@ag.ds-next/react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ChrisLaneAU ChrisLaneAU changed the title Table fixed columns table: fixed columns Jul 19, 2024
import { TableContext } from './TableContext';
import { useScrollerContext } from './ScrollerContext';

const MAX_FIXED_COLUMNS_FROM_LEFT = 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows us to use a future-proof API of frozenColumns?: number[]; instead of something like isFirstColFrozen, isLastColFrozen.

@ChrisLaneAU ChrisLaneAU changed the title table: fixed columns table: frozen columns Jul 19, 2024
return {
[minContainerBreakpointForFrozenColumns]: {
...acc[minContainerBreakpointForFrozenColumns],
[`& :nth-child(${columnNumber}):where(td, th)`]: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emotion complains about nth-child but it seems fine as no style tags are inserted and the component can't be customised.

css={{
position: 'relative',
width: '100%',
<ScrollerContext.Provider
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of the changes in this file are just due to this context wrapper.

@ChrisLaneAU ChrisLaneAU marked this pull request as ready for review July 19, 2024 03:44
Copy link
Contributor

github-actions bot commented Jul 19, 2024

PR Preview Action v1.4.7
🚀 Deployed preview to https://agriculturegovau.github.io/agds-next/pr-preview/pr-1720/
on branch gh-pages at 2024-07-30 07:01 UTC

Comment on lines +23 to +25
throw Error(
'ScrollerContext not found. If using a Table, ensure it is wrapped with <TableWrapper>.'
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little concerned by this because I'm not confident that all consumers are currently using the <TableWrapper> 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, have just checked and it's not always used…

@@ -23,6 +32,8 @@ export function TableScroller({ children }: TableScrollerProps) {
const [thumbPosition, setThumbPosition] = useState(0);
const [thumbWidthRatio, setThumbWidthRatio] = useState(0);
Copy link
Contributor

@stowball stowball Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your doing, but just noticed lots of flash of scrollbars disappearing on mount, so we should assume that it doesn't have scrollbars until it does. Might even be worth adding a comment to that effect

Suggested change
const [thumbWidthRatio, setThumbWidthRatio] = useState(0);
const [thumbWidthRatio, setThumbWidthRatio] = useState(1);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah ok, I'll look into this. Knowing not everyone has the TableWrapper, I think we'll need a different approach to this scrollbar + overlay with frozen columns functionality.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that scrollbar should still just be in TableWrapper, so that way, people who use it get the benefits of the scrollbar and those that don't, don't, but they may have their own solution or none is needed.

The other stuff should be moved out though.

If this isn't technically possible, then we'll have to revert TableWrapper to what it used to be and move the scroller elsehwere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the scrollbar needs the overlay according to our designs. The overlay doesn't need x-axis offsets if there's no frozen columns. So, I think it'll be all fine to do that part. If we can put the scroller inside the Table, then we don't need the extra context I added. Just need to look into it more.

Comment on lines 251 to 252
background: boxPalette.borderMuted,
width: '1px',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be applying this border if there are no frozen columns
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that? Doesn't it signify the hard edge of scroll, as opposed to the frozen column?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. I think it only signifies the edge of a fixed column that isn't scrolled. The table should keep its "blank" edges in its normal 0 or fully scrolled states

Comment on lines 299 to 300
background: boxPalette.borderMuted,
width: '1px',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, I don't think this should be applied if the column isn't frozen
image

...(scrollerHeight && { height: `${scrollerHeight / 16}rem` }),

// Switch between single line at full scroll and overlay shadow at partial scroll
...(scrollerRef.current?.scrollLeft === 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should transition these shadows so they don't appear abruptly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • transition overlays

@stowball
Copy link
Contributor

I haven't reviewed the code yet, but it does all seem to function remarkably well 💪

@ChrisLaneAU ChrisLaneAU changed the base branch from main to audit-2024 July 29, 2024 06:58
@ChrisLaneAU ChrisLaneAU changed the base branch from audit-2024 to main July 29, 2024 06:59
Copy link

sonarcloud bot commented Jul 30, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
30.1% Duplication on New Code (required ≤ 30%)

See analysis details on SonarCloud

@stowball stowball marked this pull request as draft September 3, 2024 03:25
@stowball stowball changed the base branch from main to develop September 12, 2024 04:57
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.

2 participants