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

Add horizontal scroll indicator feature #814

Merged
merged 19 commits into from
Jan 8, 2021

Conversation

kpfefferle
Copy link
Member

@kpfefferle kpfefferle commented Mar 12, 2020

Adds a new feature to optionally enable horizontal scroll indicator gradients on either side if there is scrollable overflow extending in that direction:

scroll-indicators

The scroll indicator gradients respect changes to the size of the ember-table container and table both, and they will render inside fixed columns if they are present. Includes documentation page demonstrating the feature and tests exercising the feature.

@kpfefferle kpfefferle requested a review from a team May 5, 2020 22:06
@kpfefferle kpfefferle marked this pull request as ready for review May 5, 2020 22:10
@aschenoni
Copy link

image
Is the box shadow styling the scrollbar in cases where there's an overflow in the y direction as well?

@kpfefferle
Copy link
Member Author

@aschenoni This may be the scroll indicator rendering above the scroll bar. The scroll indicator element is a peer of the scrolling overflow container.

@aschenoni
Copy link

I wrote a test and a fix. I'll submit a PR to this branch

@aschenoni
Copy link

aschenoni commented May 6, 2020

Take a look at #821

Copy link
Member

@mixonic mixonic left a comment

Choose a reason for hiding this comment

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

Looking into it, but does this defeat the lazy rendering of horizontal content as the element with overflow changes?

addon/components/ember-table/template.hbs Show resolved Hide resolved
addon/components/ember-thead/component.js Show resolved Hide resolved
addon/components/ember-thead/component.js Show resolved Hide resolved
addon/components/-private/scroll-indicators/component.js Outdated Show resolved Hide resolved
@kpfefferle kpfefferle changed the base branch from master to 3.0-beta October 22, 2020 19:25
@@ -91,7 +91,7 @@ export default Component.extend({
return {
columns: null,
registerColumnTree: this.registerColumnTree.bind(this),
tableId: this.elementId,
tableId: `${this.elementId}-overflow`,
Copy link
Member Author

Choose a reason for hiding this comment

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

@mixonic I believe that this update should keep row and column occlusion working as before. Yes, the scrolling element has changed, but I believe that this is the reference used for what element to observe the scroll on.

@kpfefferle kpfefferle requested review from mixonic and a team October 22, 2020 20:24
Copy link
Contributor

@twokul twokul left a comment

Choose a reason for hiding this comment

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

Took a pass. Let me know if the comments make sense 😄

@@ -0,0 +1 @@
export { default } from 'ember-table/components/-private/scroll-indicators/component';
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this export?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this export, we can't render the {{-ember-table-private/scroll-indicators}} component in the {{ember-table}} template.

addon/components/-private/scroll-indicators/component.js Outdated Show resolved Hide resolved
if (this.get('enableScrollIndicators')) {
this._addListeners();
}
addObserver(this, 'enableScrollIndicators', this._updateListeners);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to set up the observer if enableScrollIndicators is false?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because we need to add the listeners if the value of enableScrollIndicators is later changed to true

Copy link
Member

@mixonic mixonic left a comment

Choose a reason for hiding this comment

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

Thanks for the excellent work and iteration on this @kpfefferle. It is a really exciting contribution, and addresses UX cases across many of our apps at Addepar.

I'll note that we've integrated this change into our (pretty large) test suites and have no regressions. We did need to adjust some tests for the DOM change.

Lets get it in and cut another beta.

@kpfefferle kpfefferle merged commit 57e7574 into 3.0-beta Jan 8, 2021
@kpfefferle kpfefferle deleted the kpfefferle/scroll-indicators branch January 8, 2021 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants