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

Fix a few minor UI bugs #126

Merged
merged 1 commit into from
Apr 15, 2014
Merged

Fix a few minor UI bugs #126

merged 1 commit into from
Apr 15, 2014

Conversation

courajs
Copy link
Contributor

@courajs courajs commented Apr 6, 2014

First off I'm pretty welcome to feedback of all sorts.
The "hack" of adding 3 px I think was from before tolerance: 'pointer' was used or available from jQuery UI. Removing it didn't do much, but it made the reorder indicator jump to the beginning of the table instead of the end when you were hovering to switch with the last column. So I looked into it, and there was a ui-state-highlight that was taking up a little bit of space. It wasn't actually being displayed, only used as a point of reference for the .ember-table-column-sortable-indicator. So I made it zero-width.
What I'm a bit less sure about is changing this.$().parent().outerWidth()); to this.$().innerWidth(). I was afraid it would create some sort of bootstrapping problem, but everything looked fine in my testing.

When using forceFillColumns, resulting table would always be
horizontally scrollable by 3px.

When dragging to reorder columns, header cell text would jump around
a bit depending on cursor position.

Total width of table was being calculated slightly incorrectly - it
was including its own borders (and potentially margin) in the
calculations.
@courajs
Copy link
Contributor Author

courajs commented Apr 6, 2014

Oh and this fixes #123

@azirbel
Copy link
Contributor

azirbel commented Apr 7, 2014

This looks great so far - thank you!

I tested this fix against an application where I was having trouble with the 3px wiggle, and it looks like it has improved the situation, but there is still a (smaller) wiggle for me. I'll be happy to merge this PR as long as it's an improvement, but that's worth looking into. Did your fix resolve the problem entirely for you? How do you have your table configured (so I can reproduce it)?

I will look at the outerWidth/innerWidth change as soon as I can, but am pretty busy for the next couple days. If anyone else has feedback in the meantime, that would be awesome.

@courajs
Copy link
Contributor Author

courajs commented Apr 7, 2014

I'm simply setting forceFillColumns=true on the simple/Hello World example in your gh_pages app.
What are the settings on the table you're still getting the wiggle for?

@azirbel
Copy link
Contributor

azirbel commented Apr 7, 2014

Hmm, in fact it seems that in my table, forceFillColumns is not working at all - I must have done something sketchy when setting it up. I tried this fix with gh_pages like you did and it works perfectly.

Give me a day or two to look into the innerWidth/outerWidth question, then we'll merge this in.

@courajs
Copy link
Contributor Author

courajs commented Apr 7, 2014

Awesome!

azirbel added a commit that referenced this pull request Apr 15, 2014
@azirbel azirbel merged commit 0f157c6 into Addepar:master Apr 15, 2014
@azirbel
Copy link
Contributor

azirbel commented Apr 15, 2014

Yep, everything looks good. Thanks @FellowMD!

@Bartheleway
Copy link

This actually create a bug when resizing the window. It always crop 3px height ...

this.set('_height', this.$().innerHeight());

should be change to

this.set('_height', this.$().outerHeight());

I guess that the previous line which is for the width should also use the outerWidth instead of innerWidth. It actually remove the wiggle on the right of the table for me.

@courajs
Copy link
Contributor Author

courajs commented Jun 5, 2014

I don't remember that clearly, but I think during testing both seemed to work so I just went with innerHeight. If you make the change I'll try both locally to see

@Bartheleway
Copy link

This is actually even worst than expected. In the case of #133 it completly breaks the scrolling when resizing the window. I guess we have to rollback or think about a better solution.

@azirbel
Copy link
Contributor

azirbel commented Jun 6, 2014

Ok, let's discuss in #133 since that's open as a bug.

@jrhe
Copy link
Contributor

jrhe commented Jun 17, 2014

The issues this PR fixed are no longer an issue on master without this commit. I think it was the antiscroll fix which did it. JSBin to verify:

http://jsbin.com/civexixo/4

@azirbel
Copy link
Contributor

azirbel commented Jun 17, 2014

The 3px wiggle is still an issue, but the problem mentioned by @Bartheleway is a bigger problem so I've reverted this change on master. I think we should try another fix where we remove the 3px padding from contentWidth (which does solve the wiggle problem) and find another workaround for reordering a column to be the rightmost column.

@azirbel
Copy link
Contributor

azirbel commented Jun 17, 2014

I've opened a new PR to discuss/improve the 3px wiggle fix: #172. It looks to me like reordering a column to be the rightmost column is already a little broken; maybe we can improve it as we address the wiggle problem.

@courajs courajs deleted the wiggle branch January 6, 2015 16:27
@courajs courajs restored the wiggle branch January 6, 2015 16:27
Gaurav0 pushed a commit to Gaurav0/ember-table that referenced this pull request Oct 13, 2015
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.

4 participants