[Table] add Responsive table option

[Skaronator]

Closes: #9221

[mbrookes]

@Skaronator This seems to have a problem at narrower widths:

However the existing implementation already works as expected, without the additional prop:

[Skaronator]

@mbrookes Fixed. Was only in the demo though.

[mbrookes]

Then it seems like that should be the default. I can't imagine a valid use-case for having the content overflow the container.

[Skaronator]

However the existing implementation already works as expected, without the additional prop:

Thats because the Demo Frame is using overflowX: 'auto' as well. It would look pretty ugly if it wouldn't do that. The only way to counter this would be to minimize the column count.

Then it seems like that should be the default. I can't imagine a valid use-case for having the content overflow the container.

Yes it should be always responsive IMO . The weird thing is Bootstrap 4 now has responsive classes for each view breakpoint. As you can see here they now have .table-responsive{-sm|-md|-lg|-xl}.

Just checked the github repo from bootstrap and looks like @pat270 added it via twbs/bootstrap#22804

[mbrookes]

Thats because the Demo Frame is using overflowX: 'auto'

Yes, that's what I was suggesting should be the default.

The weird thing is Bootstrap 4 now has responsive classes for each view breakpoint.

I'm not clear on what happens there when above the breakpoint size at which overflowX: 'auto' is applied. Is the content hidden? (doesn't seem useful); Does it messily overflow? (Doesn't seem useful.) Something else?

For the purpose of this PR, I'd suggest making overflowX: 'auto' the default, and drop any references to "responsive". There'll be no need for the demo either, so the PR will be a one-line change. 👍

[Skaronator]

I'm not clear on what happens there when above the breakpoint size at which overflowX: 'auto' is applied. Is the content hidden? (doesn't seem useful); Does it messily overflow? (Doesn't seem useful.) Something else?

No clue. I think it just overflow and break the whole layout. Pretty ridiculous if you ask me.

I've updated the PR. All tables are now responsive.

[Skaronator]

Mh the display: block destroys the table layout on desktop. I could fix that by wrapping around a <div> and do the overflow on the div but I would like to avoid that.

I'll check this later.

[Skaronator]

I still hate this. Maybe I should move the classNameProp variable and {...other} props back to <ComponentProp>?

[mbrookes]

I hadn't appreciated the need for adding a wrapper. It explains why it was part of the demo, not the default behavior. We have other components that similarly require a suitably formatted container, such as GridList.

It seems right that <Table> should create a <table>.

Can we return to the beginning? What were you trying to achieve that the current Table doesn't already do?

[Skaronator]

Can we return to the beginning? What were you trying to achieve that the current Table doesn't already do?

The current table overflows by default which is kinda bad since this is a mobile optimized framework. So I requested a responsive table option in #9221 familar to the bootstrap responsive-table thing.

I made a PR for it and then you said, that tables should be always responsive since there is no reason not to have a responsive table at all, which is true.

[mbrookes]

The current table overflows by default [...] So I requested a responsive table option

Responsive isn't really the right word for it, rather not overflowing its container. I should have looked more closely at what component the style was applied to I didn't anticipate you having to move the container into the component.

Unfortunately the current solution introduces some new issues, so while I appreciate your efforts, it may be best to leave things as they are. If you wanted to make that more explicit in the docs by mentioning that the container needs to have overflowX: 'auto' applied that mightn't be a bad thing.

[mbrookes]

Skarantor, thanks for your efforts, but I"m closing this for now.