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

[FEATURE] Support horizontal scrolling #222

Merged
merged 4 commits into from
Dec 8, 2016
Merged

[FEATURE] Support horizontal scrolling #222

merged 4 commits into from
Dec 8, 2016

Conversation

offirgolan
Copy link
Collaborator

Conditions:

  • All widths must be defined on all visible columns
  • Widths must be the same unit

@offirgolan
Copy link
Collaborator Author

@taras I would love your feedback on this one

@taras
Copy link
Collaborator

taras commented Oct 12, 2016

How complete is this implementation? I guess it just uses regular scrollbar?

@offirgolan
Copy link
Collaborator Author

Its very naive. It works well but just uses the native scrollbar.

@@ -14,13 +14,17 @@ export default Component.extend(InViewportMixin, {

didInsertElement() {
this._super(...arguments);

let scrollBuffer = this.get('scrollBuffer');
Copy link
Collaborator

Choose a reason for hiding this comment

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

should all these variables be const instead of let, since you're not reassigning the variable name? (noticed this a lot in the repo)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added Ember Suave to fix this kind of issues. It says to use let so I'm going with it 😝

Copy link
Collaborator

@alexander-alvarez alexander-alvarez Oct 21, 2016

Choose a reason for hiding this comment

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

not sure what its deal is, but there's so many variables that aren't being reassigned, that should be const
https://github.com/dockyard/styleguides/blob/master/engineering/javascript.md#assignment
😢

Offir Golan added 2 commits October 21, 2016 13:25
* master:
  chore(package): update broccoli-asset-rev to version 2.5.0 (#227)
  chore(package): update ember-data to version 2.9.0 (#230)
  chore(package): update ember-cli-mirage to version 0.2.3 (#232)
  Ember Suave + Cleanup (#234)
  Add label to base column to be used in column types (#221)
  chore(package): update ember-scrollable to version 0.3.4 (#226)
* master: (24 commits)
  chore(package): update ember-wormhole to version 0.5.1 (#261)
  Update changelog
  Released v1.6.0
  [FEATURE] Draggable Columns (#258)
  chore(package): update ember-cli-code-coverage to version 0.3.8 (#257)
  Resizable column fixes (#252)
  fix: repeated scrollToRow (#254)
  chore(package): update ember-cli-code-coverage to version 0.3.7 (#253)
  Demo Cleanup (#251)
  chore(package): update ember-lodash to version 0.0.11 (#245)
  chore(package): update ember-cli-code-coverage to version 0.3.6 (#249)
  Document `Row#content` (#250)
  Update resizable snippet
  Released v1.5.2
  [FEATURE] minResizeWidth + Event bubbling fix (#244)
  Update readme
  Update readme
  Released v1.5.1
  Add safe checks scroll logic (#241)
  [DOCFIX] Make breakpoints of demo app the same as ember-responsive's default (#240)
  ...
@gwak
Copy link

gwak commented Dec 7, 2016

Hi, any idea when this PR going to be merged into master ?

@offirgolan
Copy link
Collaborator Author

I've been waiting on @alexander-alvarez to see if he can find a workable solution using ember-scrollable. Maybe I can release this as a temporary solution until then.

@alexander-alvarez
Copy link
Collaborator

progress has been made... about to attempt to do the double scrolling - alphasights/ember-scrollable#29

@offirgolan
Copy link
Collaborator Author

@alexander-alvarez thats great to hear! Looking at the PR, it seems like your solution will take a bit of work and some time until it gets reviewed / merged. I think Im gonna go ahead and merge this so that way there is no real pressure to get a final solution in.

@offirgolan offirgolan merged commit 029535c into master Dec 8, 2016
@offirgolan offirgolan deleted the overflow-x branch December 8, 2016 00:14
@gwak
Copy link

gwak commented Dec 8, 2016

Thanks, it works great and sufficient for our need right now. I look forward to check ember-scrollable solution though.
EDIT: Actually I'll probably wait to publish the change, because our tables are in a resizable container and when the container width is superior to the table width now being fixed the layout is kinda broken.

@gwak
Copy link

gwak commented Dec 8, 2016

Actually what I eventually did is setup an event handler for scroll on the body scrollable-content div with jQuery in the didRender hook of my component. I then simply modify the scrollLeft of the .lt-head-wrap to match the scrollLeft of the ".lt-body-wrap .scrollable-content" . It works pretty well, be you need to enable overflow-x:auto on the body content and set an id to the light-table component.

@meliborn
Copy link

meliborn commented Mar 6, 2017

Does it work? I have header and footer fixed both and many th, but I can't scroll and they are overlaping each other.

@offirgolan
Copy link
Collaborator Author

@meliborn yes its works. See here

@offirgolan
Copy link
Collaborator Author

@meliborn Make sure all your columns specify a width and all width are the same unit.

@gwak
Copy link

gwak commented Mar 24, 2017

Hi @offirgolan, the horizontal scroll works fine when the element containing the table is narrower than the table, but as soon as the container is wider than the table the table doesn't fill up the container. I guess it would be nice if the table width matches the container width if the container is wider than the table width.
Would it be possible ?

@meliborn
Copy link

meliborn commented Apr 3, 2017

@offirgolan What should I do to enable horizontal scrolling? Can't find any specific code in provided example.

@meliborn
Copy link

meliborn commented Apr 4, 2017

Had to add .table-container. But there is a problem with pagination - it scrolls as well, even with fixed=true.

@offirgolan
Copy link
Collaborator Author

@meliborn can you open a new issue with some screenshots and maybe a reproduction?

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.

5 participants