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: equal behaviour for height and max-height #14660

Conversation

arthurdenner
Copy link
Contributor

Closes #13751.

This PR is an attempt to close the referenced issue since I'm not sure if it breaks something...

Since the max-height is calculated this way, if you specify it with a unit other than px (an integer), it doesn't calculate properly, therefore, doesn't show the scrollbar.

I looked into why it's calculated like that and couldn't figured out, so I tried to replace it with the same calculation used for height and it worked as expected. Even this example, created in the PR that originated the max-height feature, just worked as before.

I checked the documentation after changing the calculation and all the examples are working as before, so it seems to me that the change doesn't break anything, but please review it carefully since I'm not aware of all the edge cases covered by the current calculation.

@element-bot
Copy link
Member

Deploy preview for element ready!

Built with commit fbdd002

https://deploy-preview-14660--element.netlify.com

@ziyoung ziyoung added this to the 2.8.0 milestone Apr 4, 2019
@ziyoung ziyoung self-requested a review April 4, 2019 06:02
@ziyoung
Copy link
Contributor

ziyoung commented Apr 23, 2019

@arthurdenner It works for me.

@ziyoung
Copy link
Contributor

ziyoung commented Apr 23, 2019

I checked the commit records of Table.vue and I think it is ok to do so. This is git blame of Table.vue https://github.com/ElemeFE/element/blame/595271f46332287cfcd42385c5b9ba05927e951d/packages/table/src/table.vue#L339

@ziyoung ziyoung merged commit 9c62040 into ElemeFE:dev Apr 24, 2019
@arthurdenner arthurdenner deleted the table/equal-behaviour-for-height-and-max-height branch April 24, 2019 12:47
@zhuxianbin
Copy link

There is another bug, set the table of max-height and the height is wrong.

@askart
Copy link

askart commented Apr 25, 2019

Can confirm, all tables' heights are broken

@cjsky666
Copy link

There is another bug, only set the table of max-height and the height is wrong

@ziyoung
Copy link
Contributor

ziyoung commented Apr 28, 2019

When max-height is string, it will cause another bug. https://codepen.io/ziyoung/pen/qwvryL

@fangwenzheng88
Copy link

fangwenzheng88 commented Apr 29, 2019

There is another bug,want it back in the next release

FAKER-A pushed a commit to FAKER-A/element that referenced this pull request May 7, 2019
@zmyjs
Copy link

zmyjs commented May 13, 2019

Why did you do that, this leads to bugs

@iyow
Copy link

iyow commented May 23, 2019

set max-height then
max-height was forcibly modified to 60px
image

@gavin1990
Copy link

gavin1990 commented May 24, 2019

bodyHeight returns the default empty height in case of data loading delay.
image
image
example: https://codepen.io/robotss/pen/eaMmaY?&editable=true

@XMoonLights
Copy link

set max-height then
max-height was forcibly modified to 60px
image

I also have this problem, the max-heigth setting is successful, but the wrapper's max-heigth is fixed at 77px.

@LoseyourseIf
Copy link

I also have this problem height:'80vh' does not work ! so I used old version '2.7.2' it's ok

weisiren168 pushed a commit to weisiren168/element that referenced this pull request Jun 20, 2019
lzq4047 pushed a commit to lzq4047/element that referenced this pull request May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The [Bug Report] Table attribute max-height uses a string without scrollbars