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] Fix Table sizing when shown in flex parent #1479

Merged
merged 6 commits into from
Aug 22, 2017

Conversation

cmslewis
Copy link
Contributor

@cmslewis cmslewis commented Aug 21, 2017

Fixes #1472

Checklist

  • Include tests

Changes proposed in this pull request:

  • BEFORE: All quadrants were position: absolute, meaning the surrounding table container had no idea how big to make itself, so it collapsed down to its min-height of 60px in containers that didn't have a fixed size.

  • AFTER: Keep the MAIN-quadrant content in the document flow to ensure the table sizes itself appropriately.

Reviewers should focus on:

  • CSS nuances. Am I using flex appropriately?
  • I've found the Features (Legacy) page in the table preview helpful for testing this!
  • Please test the dev preview thoroughly to ensure nothing looks broken! (e.g. scrollbars should still not be visible in nonMAIN quadrants)

@giladgray
Copy link
Contributor

@cmslewis failed linting and scrolling unit tests... need a preview to properly review this.

@blueprint-bot
Copy link

Fix tests

Preview: documentation | table
Coverage: core | datetime

Copy link
Contributor

@themadcreator themadcreator left a comment

Choose a reason for hiding this comment

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

only thing that i really want is top: 0; bottom: 0; and an explanation about using non explicitly sized parents

right: 0;
bottom: 0;
// use `relative` to ensure the table can size itself according to the MAIN quadrant's contents alone.
// other quadrants should remain `absolute` to ensure they're taken out of the document flow.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 this is usually my go-to solution for cutting my components out of the layout flow

z-index: $table-quadrant-z-index-main;
// the MAIN quadrant should fill the whole table so that ghost cells will be visible if enabled
width: 100%;
height: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

height: 100% can be tricky sometimes. just want to make sure this works correctly.

<div>
    <div style="height: 100%">everything is cool</div>
    <div>what now?</div>
</div>

this should be okay since we control the siblings of this div, but just something to keep an eye on.

Copy link
Contributor Author

@cmslewis cmslewis Aug 22, 2017

Choose a reason for hiding this comment

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

Yep, I was cognizant of this. We're good as far as I can tell. Every other sibling is position: absolute.

right: -$table-quadrant-scroll-container-overflow;
height: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

i prefer to use top: 0; bottom: 0; with position: absolute

>
{renderColumns({}, LARGE_NUM_COLUMNS)}
</Table>,
<div style={{ height: CONTAINER_HEIGHT, width: CONTAINER_WIDTH }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

so, what happens if users DON'T put the table in an explicitly sized div? can we never use normal flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the point of this PR actually: to fix that.

BEFORE (buggy):
image

AFTER (fluid height):
image

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, excellent.

right: -$table-quadrant-scroll-container-overflow;
height: 100%;
bottom: 0;
height: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure this is entirely necessary, but won't hurt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whenever I specify both top and bottom, I like to take height out of the picture so the two schemes don't compete unpredictably.

@blueprint-bot
Copy link

Little CSS polishes

Preview: documentation | table
Coverage: core | datetime

@cmslewis cmslewis merged commit dd7d09b into master Aug 22, 2017
@cmslewis cmslewis deleted the cl/table-flex-height-fix branch August 22, 2017 23:02
cmslewis added a commit that referenced this pull request Aug 22, 2017
* Style fixes when table is within a display: flex container

* Remove unnecessary rules

* Clearer comment

* Try to fix lint

* Fix tests

* Little CSS polishes
This was referenced Aug 22, 2017
cmslewis added a commit that referenced this pull request Aug 23, 2017
* [Table] Fix Table sizing when shown in flex parent (#1479)

* Style fixes when table is within a display: flex container

* Remove unnecessary rules

* Clearer comment

* Try to fix lint

* Fix tests

* Little CSS polishes

* Prepare Release 1.25.2

* fix normalize.css version on docs site (#1478)

* fix normalize.css version in site-docs
* rebuild site-docs.css

* Rebuild

* [Table] Fix for user-land sass compilation (#1483)
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.

4 participants