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] feat(Table2): new 'scroll' instance method #5968

Merged
merged 39 commits into from
Mar 6, 2023

Conversation

ccullotta
Copy link
Contributor

@ccullotta ccullotta commented Feb 23, 2023

Fixes #5915

Enables functionality which can address this scroll issue

Checklist

  • [n/a] Includes tests
  • Update documentation

Changes proposed in this pull request:

  • Add a new instance method Table2.prototype.scroll() which allows users to manually scroll the table by a given left / top offset in pixels
  • Demonstrate usage of this new method in table-dev-app
  • Provide an API to display a visual indicator that shows the user an affordance for auto-scrolling

Reviewers should focus on:

If this change makes sense as a "enable-new-feature" solution to the above issue, vs changing default table behavior. Also that the new "scroll" function behavior makes sense for general context.

Screenshot

2023-03-01 12 53 16

@ccullotta ccullotta marked this pull request as draft February 23, 2023 18:10
@adidahiya
Copy link
Contributor

lint fix, also added some basic doc strings

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor

ran prettier

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya adidahiya changed the title added 'scroll' function to table2 properties to enable outside scroll modifications [WIP] [DRAFT] [table] feat(Table2): new 'scroll' instance method allows external scroll modifications Feb 23, 2023
@adidahiya
Copy link
Contributor

created basic demo

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor

making demo a little nicer

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@ccullotta
Copy link
Contributor Author

GIF of the solution:
2023-02-24_17-56-23 (1)

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

In general, the idea of a Table2.prototype.scroll instance method sounds fine; it seems reasonable to add this next to the existing scrollToRegion instance method. But we'll need to do some work to make it more usable.

First off, there should be some visual indication that the user is triggering a scroll. One idea for this would be to add a linear gradient that fades from partially-transparent $black to fully-transparent $black at the top / bottom of the table's main quadrant. This could be supported with modifier classes on the <Table2> component, something like Classes.TABLE_BODY_IS_SCROLLING_{TOP|LEFT|BOTTOM|TOP}.

Secondly, the example needs to have improved scroll trigger behavior. When I move my mouse back towards the middle of the table (away from the 30% top or bottom regions), the scrolling should stop immediately. It shouldn't continue as an automatic animation.

2023-02-27 15 23 50

For documentation, you should add to the "Instance methods" section of the table docs:

@### Instance methods

packages/table/src/table2.tsx Outdated Show resolved Hide resolved
@adidahiya
Copy link
Contributor

remove unnecessary copyright

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor

small fixes

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor

removed cruft and will-change edits

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@ccullotta
Copy link
Contributor Author

Updated to fix scrolling behavior to be more responsive. Added both a cursor, and a visual overlay affect to show the user is scrolling. Interested for feedback on setScrollOverlayBackground, I initially worked it as a setScrollIndicator (left, right, top,..) function, but decided that this seemed too rigid. Perhaps this is too custom though?

@adidahiya
Copy link
Contributor

merged scroll and setScrolling indicator into setScrolling

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

nice, this is looking pretty close!

I would suggest adding this to the main mutableTable example, with a toggle to enable the behavior (titled something like "Demo programmatic scrolling API" in the sidebar)

some small polish items, if possible (might be ok to address as follow-ups):

  1. sometimes the scroll indicator sticks around after you mouse off and reach the end of the table, like at the end of this gif:

2023-03-02 11 47 17

  1. sometimes it keeps scrolling after I've moused off quickly, like this:

2023-03-02 11 47 42

packages/table/src/quadrants/tableQuadrantStack.tsx Outdated Show resolved Hide resolved
packages/table/src/table2.tsx Outdated Show resolved Hide resolved
packages/table/src/table2.tsx Outdated Show resolved Hide resolved
packages/table/src/tableState.ts Outdated Show resolved Hide resolved
packages/table/src/table2.tsx Outdated Show resolved Hide resolved
packages/table/src/table2.tsx Outdated Show resolved Hide resolved
ccullotta and others added 5 commits March 2, 2023 12:14
Co-authored-by: Adi Dahiya <adahiya@palantir.com>
Co-authored-by: Adi Dahiya <adahiya@palantir.com>
Co-authored-by: Adi Dahiya <adahiya@palantir.com>
@ccullotta
Copy link
Contributor Author

was able to fix the first issue, second one is still eluding me, but hoping we can fix that as a flup? Also merged my example into the existing with the switch being "Demo programmatic scrolling API"

@adidahiya
Copy link
Contributor

adding enum that was accidentally ommitted

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

almost there!

yes it's fine to fix that second issue as a FLUP

packages/table-dev-app/src/mutableTable.tsx Outdated Show resolved Hide resolved
packages/table-dev-app/src/mutableTable.tsx Outdated Show resolved Hide resolved
packages/table-dev-app/src/mutableTable.tsx Outdated Show resolved Hide resolved
packages/table-dev-app/src/mutableTable.tsx Outdated Show resolved Hide resolved
@adidahiya
Copy link
Contributor

removing more cruft

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@ccullotta
Copy link
Contributor Author

Copied over the instance method from table 2 into table as well, apparently it was required for the documentation to port into the instance method docs, and I assume that both should have it anyhow. Also added the documentation. There is a little funkiness with the formatting though, @adidahiya do you have any dots?

image

@ccullotta
Copy link
Contributor Author

The above is capitalized correctly in the commits I made ftr, just forgot to recompile

@adidahiya
Copy link
Contributor

making certain state properties private class properties

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

looks good in table-dev-app preview!

re: typedef in this bit of the docs... the docs-theme is a bit buggy when it comes to this... I'll try to fix it in a follow-up PR

image

packages/table/src/docs/table-api.md Outdated Show resolved Hide resolved
packages/table/src/table.tsx Outdated Show resolved Hide resolved
@adidahiya
Copy link
Contributor

use table2 for documentation string

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya adidahiya merged commit e5bff5a into develop Mar 6, 2023
@adidahiya adidahiya deleted the cc/enable-scroll-control branch March 6, 2023 20:01
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.

Table2 doesn't detect drag events on the top of the scrolling container
2 participants