add new features to the table component #488

AlejandroYanes · 2018-12-27T18:24:50Z

fixes: #453

coveralls · 2018-12-27T18:34:57Z

Pull Request Test Coverage Report for Build 1020

42 of 72 (58.33%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-1.08%) to 76.857%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/Table/head/arrowDown.js	2	4	50.0%
src/components/Table/head/header.js	21	49	42.86%

Files with Coverage Reduction	New Missed Lines	%
src/components/Table/head/index.js	1	81.82%

Totals
Change from base Build 942:	-1.08%
Covered Lines:	1333
Relevant Lines:	1661

💛 - Coveralls

…ilesbridge/react-rainbow into improve-table

reiniergs · 2019-01-01T21:02:46Z

src/components/Column/index.js

 };

 Column.defaultProps = {
    header: undefined,
    component: undefined,
+    sortable: true,


column shouldn't be sortable by default

LeandroTorresSicilia · 2019-01-02T03:10:00Z

src/components/Table/body/cell.js

    return (
-        <td className="rainbow-table_cell">
+        <td className="rainbow-table_cell" style={{ width }}>


don't use a style object directly, instead create a const:
const cellStyle = { width };

LeandroTorresSicilia · 2019-01-02T03:10:28Z

src/components/Table/head/arrowDown.js

+};
+
+ArrowDown.defaultProps = {
+    direction: 'desc',


the default is asc

LeandroTorresSicilia · 2019-01-02T03:10:38Z

src/components/Table/head/header.js

@@ -0,0 +1,122 @@
+/* eslint-disable no-param-reassign,class-methods-use-this */


add tests for this file

LeandroTorresSicilia · 2019-01-02T03:10:50Z

src/components/Table/head/header.js

+        {
+            'rainbow-table_header--sortable': sortable,
+            'rainbow-table_header--selected': selected,
+        },


this is right but if we always put this inside the class why here it is outside?

LeandroTorresSicilia · 2019-01-02T03:11:29Z

src/components/Table/head/header.js

+                        <ArrowDown direction={sortDirection} />
+                    </RenderIf>
+                </div>
+                <div


here we need to use a input type="range" to make this accessible

LeandroTorresSicilia · 2019-01-02T03:12:04Z

src/components/Table/index.js

+    onColumnSelect(e, columnIndex) {
+        const { onSort } = this.props;
+        onSort(e, this.state.columns[columnIndex].field);
+        this.setState({ selectedColumn: columnIndex });


this make that the table rerender completely and this is not needed, you can select the cell making it focusable

LeandroTorresSicilia · 2019-01-02T03:12:28Z

src/components/Table/index.js

        const { columns } = this.state;
+        const resizedColumn = Object.assign({}, columns[columnIndex]);
+        resizedColumn.width = width;
+        columns[columnIndex] = resizedColumn;


Here you are mutating the state directly, this is prohibitive.

LeandroTorresSicilia · 2019-01-02T03:12:46Z

src/components/Table/index.js

+        const resizedColumn = Object.assign({}, columns[columnIndex]);
+        resizedColumn.width = width;
+        columns[columnIndex] = resizedColumn;
+        this.setState({ columns });


Here when you resize the table is completely rerendered, we need to find a way to optimize this. I think you can change only the width of the header column without recreate the table.

…ader

# Conflicts: # src/components/Table/head/header.js # src/components/Table/head/index.js

LeandroTorresSicilia · 2019-01-03T19:33:07Z

src/components/Column/index.js

@@ -31,9 +31,14 @@ Column.propTypes = {
    * for a column that want to represent names on a collection of people.
    */
    field: PropTypes.string.isRequired,
+    /** Sets whether the column should control the sorting order of the data */
+    sortable: PropTypes.bool,
+    width: PropTypes.string,


add description:
Specifies the width of a column in pixels and makes the column non-resizable.
and change prop type to number.

LeandroTorresSicilia · 2019-01-03T19:33:13Z

src/components/Table/body/cell.js

@@ -18,9 +18,11 @@ CellValue.defaultProps = {
    value: undefined,
 };

-export default function Cell({ component, value }) {
+export default function Cell({ component, value, width }) {
+    const cellStyle = { width };


discard all this changes since width is not longer needed here

LeandroTorresSicilia · 2019-01-03T19:33:17Z

src/components/Table/body/row.js

-    const cells = columns.map(({ component, field }) => (
-        <Cell key={uniqueId('cell')} component={component} value={data[field]} />
+    const cells = columns.map(({ component, field, width }) => (
+        <Cell key={uniqueId('cell')} component={component} value={data[field]} width={width} />


Same ^^. Discard changes.

LeandroTorresSicilia · 2019-01-03T19:33:37Z

src/components/Table/head/index.js

+    constructor(props) {
+        super(props);
+        this.state = {
+            columnWidths: props.columns.map(column => column.width),


what is the point of this?
it is only for change the column width?
if it is, then is better to not send a resize event to Head and instead change the width directly in the Header component with a width state.

LeandroTorresSicilia · 2019-01-03T19:33:42Z

src/components/Table/index.js

-                <thead className="rainbow-table_head">
+            <div className={this.getContainerClassNames()} style={style}>
+                <table className="rainbow-table">
+                    <thead className="rainbow-table_head">
                    <tr className="rainbow-table_header-row">


indentation

LeandroTorresSicilia · 2019-01-03T19:33:47Z

src/components/Table/styles.scss

 @import "../../styles/colors";
 @import "../../styles/font-sizes";
+@import "../../styles/paddings";
+


this css file will become too huge, so we need to put the head specific styles in another css file inside the head folder, and the same for body.

LeandroTorresSicilia · 2019-01-03T19:36:11Z

you forgot to add the input type range
also add the following props to Table:

resizeColumnDisabled
minColumnWidth (default to 50)
maxColumnWidth(default to 1000)

By default, columns are resizable. Users can click and drag the width to a minimum of 50px and a maximum of 1000px, unless the default values are changed. To change the minimum and maximum width column, use the minColumnWidth and maxColumnWidth props.
You can disable column resizing for all columns by setting the datatable's resizeColumnDisabled prop to true. You can disable column resizing for a specific column by setting the width column prop.

LeandroTorresSicilia · 2019-01-04T22:17:08Z

src/components/Table/head/header.js

@@ -13,6 +13,21 @@ export default class Header extends Component {
        this.handleMouseUp = this.handleMouseUp.bind(this);
        this.headerContainer = React.createRef();
        this.resizeBar = React.createRef();
+        this.state = {
+            width: this.props.width,


here you can use:
props.width

LeandroTorresSicilia · 2019-01-04T22:17:16Z

src/components/Table/head/header.js

-        document.addEventListener('mousemove', this.handleMouseMove);
-        document.addEventListener('mouseup', this.handleMouseUp);
+        const { resizeColumnDisabled } = this.props;
+        const isResizable = !resizeColumnDisabled || this.state.width === undefined;


LeandroTorresSicilia · 2019-01-04T22:17:18Z

src/components/Table/head/header.js

+        const { resizeColumnDisabled } = this.props;
+        const isResizable = !resizeColumnDisabled || this.state.width === undefined;
+        if (isResizable) {
+            this.startXPosition = dragEvent.clientX;


we need to declare:
this.newXPosition = 0;
because if I click de resize bar without move it this variable is never declared and then in handleMouseUp when:
const width = headerContainerWidth + this.newXPosition;
we got NaN since this.newXPosition is undefined
Also we need to declare it here because whenever we click the resize bar we need to reset this value to 0 since if we move the resize bar 30px to right then this.newXPosition get 30 and when we press the resize bar again without move, it will resize the column with 30 and we don't want this.

LeandroTorresSicilia · 2019-01-04T22:17:25Z

src/components/Table/head/header.js

+            minColumnWidth,
+            maxColumnWidth,
+        } = this.props;
+        const headerStyles = { width: `${width}px` };


you can use width directly:
const headerStyles = { width };

…abled

AlejandroYanes added 3 commits December 27, 2018 13:01

feat: added sorting, the headers fixed at the top and column resizing

8904bdb

fix: style-lint errors

e59ec69

test: fixed tests

4670128

AlejandroYanes added 5 commits December 27, 2018 13:36

fix: disabled style-lint rule for descendants

8acd648

Merge branches 'improve-table' and 'master' of https://github.com/90m…

de1abd2

…ilesbridge/react-rainbow into improve-table

fix: added column resize guideline

1ff0c80

fix: added column resize limits

eb341a6

fix: add overflow hidden to wrapper

672d327

reiniergs reviewed Jan 1, 2019

View reviewed changes

LeandroTorresSicilia added 2 commits January 1, 2019 21:42

fix: add some fixes

99c7d33

fix: fix test

589c383

LeandroTorresSicilia reviewed Jan 2, 2019

View reviewed changes

src/components/Table/head/arrowDown.js

};

ArrowDown.defaultProps = {

direction: 'desc',

Copy link

Member

LeandroTorresSicilia Jan 2, 2019

Choose a reason for hiding this comment

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

the default is asc

LeandroTorresSicilia reviewed Jan 2, 2019

View reviewed changes

AlejandroYanes added 6 commits January 2, 2019 15:03

fix: removed second table

9eff520

fix: removed sorting logic, passed resizing logic to be handled by he…

fc493eb

…ader

fix: removed onSort prop from table

f7d0290

fix: done changes asked in review

c451672

fix: fixed tests

5d9e4c8

Merge remote-tracking branch 'origin/improve-table' into improve-table

4292f18

# Conflicts: # src/components/Table/head/header.js # src/components/Table/head/index.js

LeandroTorresSicilia reviewed Jan 3, 2019

View reviewed changes

fix: done changes asked in review, added first row cell as th

a3a6dd1

LeandroTorresSicilia reviewed Jan 4, 2019

View reviewed changes

LeandroTorresSicilia added 2 commits January 4, 2019 17:19

fix: add some fixes to header

0507a99

fix: add accessibility attributes and hide resize bar when resize dis…

b84d016

…abled

LeandroTorresSicilia approved these changes Jan 5, 2019

View reviewed changes

LeandroTorresSicilia merged commit 0acfc73 into master Jan 5, 2019

TahimiLeonBravo deleted the improve-table branch September 23, 2019 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add new features to the table component #488

add new features to the table component #488

AlejandroYanes commented Dec 27, 2018

coveralls commented Dec 27, 2018 •

edited

Loading

reiniergs Jan 1, 2019

LeandroTorresSicilia Jan 2, 2019

LeandroTorresSicilia Jan 2, 2019

LeandroTorresSicilia Jan 2, 2019

LeandroTorresSicilia Jan 2, 2019

LeandroTorresSicilia Jan 2, 2019

LeandroTorresSicilia Jan 2, 2019

LeandroTorresSicilia Jan 2, 2019

LeandroTorresSicilia Jan 2, 2019

LeandroTorresSicilia Jan 3, 2019

LeandroTorresSicilia Jan 3, 2019

LeandroTorresSicilia Jan 3, 2019

LeandroTorresSicilia Jan 3, 2019

LeandroTorresSicilia Jan 3, 2019

LeandroTorresSicilia Jan 3, 2019

LeandroTorresSicilia commented Jan 3, 2019

LeandroTorresSicilia Jan 4, 2019

LeandroTorresSicilia Jan 4, 2019

LeandroTorresSicilia Jan 4, 2019

LeandroTorresSicilia Jan 4, 2019

		@@ -0,0 +1,122 @@
		/* eslint-disable no-param-reassign,class-methods-use-this */

add new features to the table component #488

add new features to the table component #488

Conversation

AlejandroYanes commented Dec 27, 2018

coveralls commented Dec 27, 2018 • edited Loading

Pull Request Test Coverage Report for Build 1020

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeandroTorresSicilia commented Jan 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 27, 2018 •

edited

Loading