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

[WIP] Fixed columns #131

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cball
Copy link

@cball cball commented Jul 30, 2016

This has a decent amount more for me to do before I'd consider it mergeable but I wanted to open it up for discussion.

This library is gaining traction, so I wanted to look at adding some of the high level features developed in https://github.com/cball/justa-table so that I can put resources towards this library instead. We currently have 3 projects that need fixed column support and I know others that have expressed the need for that feature as well. @taras had concerns using fixed columns in #114. While complicated to implement, it is a pretty common ask for complex tables and I don't think you can always just implement a different UI.

First, the largest issue: Trackpad Scroll Emulator which is used by ember-scrollable can only scroll 1 way. (see https://github.com/jnicol/trackpad-scroll-emulator#5-limitations). As such, in this PR when there are fixed columns present, native scrollbars are used instead. I'd actually argue to put the work in to make native scrollbars work across the board but that's a separate issue.

TODOs before I think this is ready:

  • fix and support columnGroups
  • fix ember-infinity
  • add missing tests for new functionality
  • reduce duplication (currently a decent amount of copy/paste) to set up the columns
  • figure out a better way to organize the fixed column specific code
  • general cleanup

screen recording 2016-07-29 at 09 10 pm

most of layout done.

TODO:
- [ ] fixed/standard wrapper for headers
- [ ] remove inline css
- [ ] remove style warnings
- [ ] fix ember-infinity
@offirgolan
Copy link
Collaborator

@cball This is.. this is awesome! Im super excited for this. I think we are gonna have to make the font size on the navbar smaller to fit all these features in 😏

From a quick glance, I noticed that you're branch is a bit behind master. I would suggest to rebase since it has a lot of changes that resolve some critical issues and that will prolly conflict with some of your code.

In terms of the trackpad scroll emulation, I think @taras would be your man.

@taras
Copy link
Collaborator

taras commented Jul 30, 2016

@cball this is awesome!

I ended up adding Trackpad Scroll Emulation because we have fixed headers and footers which are setup as separate tables. With regular scrollbars, the columns in the header didn't line up with the body because they had different widths, caused by scrollbar being added to the body.

I'd love a simpler solutions. It'd mean that we'd be able to remove a dependency and reduce complexity. If you have any suggests in regards to how to manage the content width issue caused by body scrollbar, I'd love to hear them.

@cball
Copy link
Author

cball commented Jul 30, 2016

Thanks! I'll rebase and get this finished / cleaned up.

In the first pass of this feature, I want to keep the existing virtual scroll behavior in tables without fixed columns. We should be able to normalize the scroll behaviors in all tables with a followup PR. tldr there I think will be measuring scrollbars and explicitly setting widths.

We should also be able to simplify the codebase a bit in the future by making all tables without fixed columns operate the same as the standard columns from the fixed version.

@taras
Copy link
Collaborator

taras commented Jul 30, 2016

tldr there I think will be measuring scrollbars and explicitly setting widths.

I'm concerned about this. So far, I've been very intentional about not getting into a situation where we're managing dimensions of elements through state and rely on the browser to size elements appropriately.

Mixing state managed sizes and DOM sizes quickly leads to having to manage all sizes through state. This is part of the reason why we're using ember-element-resize-detector. Instead of forcing nested element to have a height, we're detecting the size of the parent and resizing the child as necessary.

I would prefer for us to continue to evolve this pattern rather than getting into setting widths and heights via state. I'm going to work on a prototype that has nested ember-scrollable components. If I can get that to work, then I'll try to build a prototype that shows how we might be able to implement this feature.

My other concern is that collapsible rows will not work with fixed columns(without doing some complex height management). This is the reason for my comment in #114. I was hoping we'd be able to use something like ember-collection where we could create a grid that was fixed columns aware. ember-collection never materialized unfortunately.

@taras
Copy link
Collaborator

taras commented Jul 30, 2016

It doesn't look like nested ember-scrollable approach is going to work. I might have to make ember-scrollable support vertical and horizontal scrollbars.

I'll keep you posted on my progress.

In the meantime, I'm curious what your thoughts are regarding collapsible rows and fixed columns.

@cball
Copy link
Author

cball commented Aug 1, 2016

I'm concerned about this. So far, I've been very intentional about not getting into a situation where we're managing dimensions of elements through state and rely on the browser to size elements appropriately.

Ember scrollable is already measuring the scrollbar and setting widths/heights, not sure how is this different?

Fixed columns are implemented using 2 absolute positioned containers with separate tables, as such they need widths and heights set.

I can certainly use ember-element-resize-detector to set the height of the absolutely positioned containers, but will still need to set the fixed column container width based on the sum of all fixed columns, and set the margin-left of the standard columns equal to that same amount. I don't see a way around explicitly setting that.

Re: collapsable rows, yes we'll probably have to do some height management so things don't get out of sync. I think I'd probably start by firing an action from the lt-spanned-row component when it has finished loading its content and has a height. Row is already an object proxy, so perhaps we can drive the syncing of height that way. I'll can update my dummy app example to include that and explore what makes sense.

High level question: Should an expanded row take on the width of the entire table (both fixed and standard columns) or just the non-fixed side?.

Agree its too bad about ember-collection, I was looking forward to that as well.

@nmcclay
Copy link
Contributor

nmcclay commented Sep 30, 2016

This is a very exciting feature, this behavior could be very useful in regard to expanding the responsive design aspects of this table component.

@RobbieTheWagner
Copy link
Member

Where does this feature currently stand? I'd be glad to help push it along

@cball
Copy link
Author

cball commented Dec 20, 2016

@rwwagner90 I was waiting on feedback on how to proceed. I unfortunately don't have any free cycles at the moment, but if this is a direction that @offirgolan and @taras would like to go with this library (I'm unsure if it is) then help moving things along would be greatly appreciated.

@Prabhakar-Poudel
Copy link

Is there any progress with the PR? I have been waiting for this feature to be merged.

@RobbieTheWagner
Copy link
Member

@offirgolan @taras can you guys weigh in here please? I would be glad to help with implementing this, but do not want to put effort in, if it is not something that is desired for this addon.

@taras
Copy link
Collaborator

taras commented Jan 9, 2017

This PR has been in a bit of standstill mostly because we're not sure the best way to proceed. We would like for this feature to happen, but we weren't 100% what approach to take. @cball proposed a solution, but we inadvertently blocked him by not providing a path for moving forward.

@alexander-alvarez has a PR in the works for ember-scrollable that'll make it possible to have horizontal and vertical scrollbars on a block. Once this lands, it might unblock this PR and provide a way to implement this feature without making the code a lot more complicated.

Would you be interested in picking this up when that PR lands?

Primarily, we need to figure out how the solution proposed in this PR is affected by that change. ember-collection is also becoming much more robust and approaching 1.0. Now might be a good time to see if we can use it to implement this feature.

What do you think?

@Prabhakar-Poudel
Copy link

has there been there any progress with this?

@gspain
Copy link

gspain commented Aug 30, 2018

It looks like the ember-scrollable PR has been merged. After a year, is there any news on this feature?

@Techn1x
Copy link

Techn1x commented May 20, 2019

@cball @taras @offirgolan I'm guessing this PR will probably need a rebase and some reworking, but now that ember-scrollable has what is needed, are there any blockers left to getting this PR in?

I think it would be a very popular feature - tables being responsive and usable at small viewports is a huge deal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Road to 4.0
Discussion
Development

Successfully merging this pull request may close these issues.

None yet

8 participants