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.

Sign up for GitHub

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

Jump to bottom

[Table] add Responsive table option #9268

Closed
wants to merge 11 commits into from
Closed

[Table] add Responsive table option #9268

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
33dbab4
Height should be set to 100% to allow scrolling
Skaronator Sep 14, 2017
07f57f2
Merge remote-tracking branch 'upstream/v1-beta' into v1-beta
Skaronator Nov 20, 2017
4b6fe46
[Table] Add responsive table
Skaronator Nov 22, 2017
2707335
Fix overflow in demo
Skaronator Nov 22, 2017
415de90
Revert "Fix overflow in demo"
Skaronator Nov 26, 2017
67ee95c
Revert "[Table] Add responsive table"
Skaronator Nov 26, 2017
15bfada
[Table] make all Tables responsive
Skaronator Nov 26, 2017
16965ae
Use wrapper
Skaronator Nov 26, 2017
8132c20
Fix errors and run docs:api
Skaronator Nov 26, 2017
f99af4b
Fix Errors
Skaronator Nov 26, 2017
794a5fc
Modify Errors to new div wrapper
Skaronator Nov 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/Table/Table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,8 @@ class Table extends React.Component<ProvidedProps & Props> {
const divWrapper = classNames(classes.wrapper, classNameProp);

return (
<div className={divWrapper}>
<ComponentProp className={classes.table} {...other}>
{children}
</ComponentProp>
<div className={divWrapper} {...other}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@mbrookes mbrookes Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

<ComponentProp className={classes.table}>{children}</ComponentProp>
</div>
);
}
Expand Down
8 changes: 7 additions & 1 deletion src/Table/Table.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,13 @@ describe('<Table />', () => {
it('should render children', () => {
const children = <tbody className="test" />;
const wrapper = shallow(<Table>{children}</Table>);
assert.strictEqual(wrapper.childAt(0).childAt(0).equals(children), true);
assert.strictEqual(
wrapper
.childAt(0)
.childAt(0)
.equals(children),
true,
);
});

it('should define table in the child context', () => {
Expand Down