feat: DataTable component #446

jaulz · 2018-07-09T17:01:50Z

Motivation

New component Data Table according to https://material.io/design/components/data-tables.html.

Test plan

callstack-bot · 2018-07-09T17:03:45Z

Hey @jaulz, thank you for your pull request 🤗. The documentation from this branch can be viewed here. Please remember to update Typescript types if you changed API.

gawrysiak · 2018-07-09T18:32:16Z

src/components/DataTable/DataTableCell.js

+  /**
+   * Content of the `DataTableCell`.
+   */
+  children: string | React.Node,


can be just React.Node (https://flow.org/en/docs/react/types/#toc-react-node)

gawrysiak · 2018-07-09T18:34:47Z

src/components/DataTable/DataTableCell.js

+
+import React from 'react';
+import { StyleSheet, TouchableOpacity, View } from 'react-native';
+import Text from './../Typography/Text';


'../Typography/Text' should be enough. Same for other imports. That's minor though :)

gawrysiak · 2018-07-09T18:38:14Z

src/components/DataTable/DataTablePagination.js

+    }
+  }
+
+  render(): Object {


is the Object type here needed?

gawrysiak · 2018-07-09T18:41:52Z

src/components/DataTable/DataTablePagination.js

+   * Customize the rows per page label. Invoked with a `{ from, to, count, page }`
+   * object.
+   */
+  labelRowsPerPage?: () => mixed,


string here maybe?

gawrysiak · 2018-07-09T18:42:33Z

src/components/DataTable/DataTable.js

+import DataTableRow from './DataTableRow';
+
+type Props = {
+  style?: any,


missing children prop

gawrysiak · 2018-07-09T18:42:50Z

src/components/DataTable/DataTableBody.js

+import { View } from 'react-native';
+
+type Props = {
+  style?: any,


missing children prop

gawrysiak · 2018-07-09T18:44:56Z

src/components/DataTable/DataTableHeaderCell.js

@@ -0,0 +1,141 @@
+/* @flow */
+
+import React from 'react';


try import * as React, breaks flow otherwise

gawrysiak · 2018-07-09T18:45:20Z

src/components/DataTable/DataTableCell.js

@@ -0,0 +1,94 @@
+/* @flow */
+
+import React from 'react';


try import * as React, breaks flow

gawrysiak · 2018-07-09T18:46:09Z

src/components/DataTable/DataTableHeaderCell.js

+    size: 1,
+  };
+
+  constructor(props) {


flow wants props: Props here

gawrysiak · 2018-07-09T18:46:43Z

src/components/DataTable/DataTableHeaderCell.js

+    });
+    const icon = sortDirection ? (
+      <Animated.View style={[styles.icon, { transform: [{ rotate: spin }] }]}>
+        <Icon source="arrow-downward" size={16} />


color is required for the icon

gawrysiak · 2018-07-09T18:47:37Z

src/components/DataTable/DataTableRow.js

+import { grey100 } from './../../styles/colors';
+
+type Props = {
+  /**


missing children prop

jaulz · 2018-07-09T18:50:36Z

@gawrysiak thanks for the comments! Will do the changes and update you. Sorry for the flow issues but I had to skip them locally since it runs into an infinite loop locally. Any idea why this is the case?

gawrysiak · 2018-07-09T19:17:43Z

@jaulz yeah, other than the comments, there's quite a long list of reported flow issues. Visually looks nice though. @satya164 can probably give you some more API feedback.

As for the flow issue, it's worth a try to run flow through the local node_modules instead of the globally installed one. It should be possible with most of the plugins, e.g. VSC:

or Nuclide:

Then look into your processes and kill any flow servers that shouldn't be running (when editor is off).

As for the terminal, normally killing the global process and running yarn flow should do.

jaulz · 2018-07-10T09:08:46Z

Okay, fixes are in. Though there is one left regarding theming, e.g.:

Cannot create DataTable.HeaderCell element because property theme is missing in props [1] but exists in Props [2].

Any idea why this occurs? Compared my component to the existing ones and can't find the difference... shouldn't withTheme provide the theme parameter?

satya164 · 2018-07-10T11:32:20Z

Not sure. Will need to check later.

ferrannp · 2018-07-27T11:11:12Z

How is this PR going @satya164 ?

jaulz · 2018-09-10T16:49:47Z

@satya164 just pushed an update so it's properly passing the build

giautm · 2018-09-20T14:13:39Z

Waitting for it. 👍

giautm

Need to remove {}

giautm · 2018-09-25T06:26:11Z

src/components/DataTable/DataTableCell.js

+            {children}
+          </Text>
+        ) : (
+          { children }


There is a bug, {} should not used right here

@Trancever :this bug was not resolved yet, why you accept the PR?

Will check it tomorrow 😊 it's anyway not yet merged...

jaulz · 2018-09-25T07:09:49Z

@giautm thanks for the hint! Though, I would rather wait for feedback from the team before I do any more changes...
@ferrannp @satya164 any comments from your side? I would be happy to incorporate any requested changes.

giautm · 2018-09-25T07:13:12Z

@jaulz : no, {} make an object with property children. That will cause:

        {typeof children === 'string' || typeof children === 'number' ? (
          <Text
            style={[
              styles.cell,
              numeric ? styles.numeric : styles.text,
              this.props.style,
            ]}
            numberOfLines={1}
          >
            {children}
          </Text>
        ) : (
          children // <---- no need `{}` right here.
        )}

Trancever · 2018-09-25T07:23:25Z

src/components/DataTable/DataTableCell.js

+            {children}
+          </Text>
+        ) : (
+          { children }


@giautm is right. We should get rid of that curly braces in this case.

Sorry, actually I meant to wait for feedback for this PR in general. As long as I don't know if this PR will be considered in this way at all it does not make sense to fix issues, does it?
Once I receive green light from your side, I am more than happy to fix issues...

Ok, I understand :) For me this PR looks great, I like the api of components. The only thing that those components misses for me are styles based on theme. There is no way to change theme to dark, we have to pass style prop to every component, am I right?

@satya164 Could you take a look on this PR?

giautm · 2018-09-25T09:12:29Z

src/components/DataTable/DataTablePagination.js

+
+  displayedRows: {
+    marginLeft: 30,
+    marginRight: 44,


I think we should not use the fixed margin like this. That cause some render issues on the device with the small screen width. (ex: iPhone SE.)

giautm · 2018-09-25T09:54:02Z

src/components/DataTable/DataTablePagination.js

+    return (
+      <View style={[styles.container, this.props.style]}>
+        <Text style={[styles.text, styles.rowsPerPage]} numberOfLines={1}>
+          {labelRowsPerPage} {rowsPerPage}


Should remove the spacing between {labelRowsPerPage} and {rowsPerPage}. And set default props of labelRowsPerPage to 'Rows per page: '

Or simply use the custom render right here. 😄

ferrannp · 2018-11-09T13:19:48Z

What's the status of this one @jaulz ?

jaulz · 2018-11-09T15:43:28Z

@ferrannp if @satya164 could take a look at the component first and confirms then I could adapt the component to the dark theme as well.

satya164 · 2018-11-22T13:31:15Z

I pushed some changes in https://github.com/callstack/react-native-paper/tree/%40satya164/data-table, but it still needs typescript definitions. ANybody up for adding the defs?

jaulz · 2018-11-23T08:01:49Z

@satya164 will take it up next week, thanks!

jaulz · 2018-11-26T14:44:59Z

@satya164 types are created in your branch. shall we create a separate PR or merge the changes in this one here?

satya164 · 2018-11-26T17:04:46Z

@jaulz thanks! merging the changes to this branch works for me :)

jaulz · 2018-11-27T12:43:44Z

@satya164 thanks! Though, I just created another PR #684 which I was working on in parallel and allows not only text and numbers in cells.

jaulz force-pushed the patch-7 branch 2 times, most recently from 5e819e4 to 3d79fac Compare July 9, 2018 17:27

gawrysiak reviewed Jul 9, 2018

View reviewed changes

src/components/DataTable/DataTablePagination.js Outdated

}

}

render(): Object {

Copy link

Collaborator

gawrysiak Jul 9, 2018

Choose a reason for hiding this comment

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

is the Object type here needed?

gawrysiak reviewed Jul 9, 2018

View reviewed changes

src/components/DataTable/DataTableHeaderCell.js Outdated

size: 1,

};

constructor(props) {

Copy link

Collaborator

gawrysiak Jul 9, 2018

Choose a reason for hiding this comment

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

flow wants props: Props here

gawrysiak reviewed Jul 9, 2018

View reviewed changes

jaulz force-pushed the patch-7 branch 3 times, most recently from b57352b to c5d4eb9 Compare July 10, 2018 09:03

ferrannp assigned satya164 Jul 27, 2018

satya164 changed the base branch from material-next to master September 2, 2018 18:59

jaulz force-pushed the patch-7 branch 3 times, most recently from c5d4eb9 to 169a8dd Compare September 7, 2018 06:27

giautm suggested changes Sep 25, 2018

View reviewed changes

Trancever suggested changes Sep 25, 2018

View reviewed changes

giautm suggested changes Sep 25, 2018

View reviewed changes

satya164 force-pushed the master branch 4 times, most recently from 1fed58c to 1cf7ae9 Compare October 11, 2018 12:28

feat: DataTable component

ddbbdc8

fix: add types for DataTable

420c1cd

Trancever approved these changes Nov 26, 2018

View reviewed changes

jaulz force-pushed the patch-7 branch from a600f3b to 420c1cd Compare November 27, 2018 09:43

satya164 merged commit 68a5b78 into callstack:master Nov 27, 2018

feat: DataTable component #446

feat: DataTable component #446

Conversation

jaulz commented Jul 9, 2018

Motivation

Test plan

callstack-bot commented Jul 9, 2018 • edited Loading

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

jaulz commented Jul 9, 2018

gawrysiak commented Jul 9, 2018

jaulz commented Jul 10, 2018

satya164 commented Jul 10, 2018

ferrannp commented Jul 27, 2018

jaulz commented Sep 10, 2018

giautm commented Sep 20, 2018

giautm left a 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

jaulz commented Sep 25, 2018

giautm commented Sep 25, 2018

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

ferrannp commented Nov 9, 2018

jaulz commented Nov 9, 2018

satya164 commented Nov 22, 2018

jaulz commented Nov 23, 2018

jaulz commented Nov 26, 2018

satya164 commented Nov 26, 2018

jaulz commented Nov 27, 2018

callstack-bot commented Jul 9, 2018 •

edited

Loading