Adding examples using context #80

gforge · 2016-11-17T10:36:14Z

See issue #76. Here are examples of the filter and pagination table.

Description

Examples with context have Ctxt prepended.
There is a HOC helper with the data context used in both examples.
There is a CtxtCell helper with the context specific cells
The Constants exports.ExamplePages now has a fileName that is concatenated with the EXAMPLES_LOCATION_BASE for generating the file element
The ExamplePage has now the two additional elements (I tried to autogenerate the EXAMPLE_COMPONENTS but it didn't work out as I hoped.

Motivation and Context

Having a data as context allows creating manipulation of the data without knowing the children. This opens up for re-using filter-functionality while keeping a simple and nice JSX formatting such as:

<FilterTable
  data=my_data
  ....>
  <Column
    columnKey="firstName"
    header={<Cell>First Name</Cell>}
    cell={<TextCell />}
  />
  ....
</FilterTable>

How Has This Been Tested?

Tested the examples in Chrome

Types of changes

[x ] New feature (non-breaking change which adds functionality)

Checklist:

My code follows the code style of this project. (ES6-style)
My change requires a change to the documentation (new examples).
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

Removed the cryptic "end"-state and _dataVersion Changd _updateData() to refresh() to better represent the function's purpose

… adding a custom PropType that only checks for the presence of the setCallback function

Moved data into a prop and created a HOCed DataTable that is used in the actual example

TODO: FilterTable should probably be a HOC as well

Moved context to Ctxt....

Added a constant value 'fileName' without the EXAMPLES_LOCATION_BASE The file is autogenerated from the EXAMPLES_LOCATION_BASE

wcjordan · 2016-11-17T14:30:38Z

examples/CtxtFilterExample.js

+
+  componentWillReceiveProps(nextProps) {
+    if (JSON.stringify(nextProps.filters) !== JSON.stringify(this.state.filters)){
+      this.setState({


Hmm converting props to state seems like an anti pattern and may cause 2 renders. Was there a reason you didn't just render off of this.props?

Since the prop is an object the values are passed as a reference and without the cloning I could not detect when the filter changed and when to update the parameter.

My comment above was directed at calling set state rather than the JSON stringify.

Yes, if I only do a comparison with the current filter prop it will already have changed since it's an object. That is why I need a cloned copy. Immutable would probably fix this

What would happen if you deleted filters from state above and deleted componentWillReceiveProps and then called this.filter() as the first line in render?

This caused >1000 warnings:

warning.js:36 Warning: setState(...): Cannot update during an existing state transition (such as within render or another component's constructor). Render methods should be a pure function of props and state; constructor side-effects are an anti-pattern, but can be moved to componentWillMount.

Your solution would be more elegant but it doesn't seem to play well with React :-(

Okay cool. Next remove the setState calls here and remove this.state.filteredData from the render method and have it instead use an object returned from the this.filter method. Then I think you should be solid.

Excellent suggestion. I've removed all the setState for all the data-wrappers and moved them into the render method.

wcjordan · 2016-11-17T14:34:36Z

examples/CtxtFilterExample.js

+            //  This can be useful when you have more complex matches
+            if (value instanceof Array) {
+              const matches = value.map(x => match(x, filters[varName])).filter(x => x === true);
+              // If all filters were identified then set this to true


this seems to work over all values not all filters. is the comment correct?

Yes, this was in case I have a single filter associated with an array of values - it's there just as an example how one could go about setting up a more complex filter.

ya, just meant the comment is misleading / should be updated to reflect that

As I changed to a for-loop I decided to remove this as well.

wcjordan · 2016-11-17T14:34:39Z

examples/CtxtFilterExample.js

+        const row = this.state.rawData.getObjectAt(index);
+
+        // Loop through all the filters and check if there's a match
+        const found = Object.keys(filters)


I can't help but thing this could be written more cleanly with reduce or maybe use _.every

I was thinking of adding lo-dash but wasn't sure if adding more dependencies into the library was a good idea.

It would be good to add it for examples, but not the library. just make sure it's under devDependencies in npm and isn't used by any code under src

The for loop solves this without adding it. I'll see if I'll need it later on.

wcjordan · 2016-11-17T14:35:52Z

examples/CtxtFilterExample.js

+            return (!match(value, filters[varName]));
+          })
+          .filter(x => x === true)
+          .length === 0;


The way this checks for no not matching filters here is a little weird to me.
Also you could gain performance if you don't run additional filters once a value fails a filter.

Yeah, this should probably be improved in some clever way. I've focused on getting it to work, a for loop with a break should be smarter. I'm just used from R to avoid all for-loops and I'm not that familiar with the ups/downs of for-loops in JS.

Ah with JS you should focus on readability. Often that's easier with functional approaches, but sometimes a loop is easier as well.

In R I think you get parallelism and other performance enhancements using a function because it drops down into compiled C to do the work. No advantages like that with JS

Again, changed to a for loop

wcjordan · 2016-11-17T14:42:25Z

examples/CtxtFilterExample.js

+      }
+
+      // Set the data filtering
+      this.state.filteredData.setIndexMap(filteredIndexes);


It doesn't seem very React-y / obeying one way data-flow to have the React lifecycle methods update the store which then updates React components...

I would recommend generating a list of filtered rows here and passing it into cells as a prop to render from (instead of the DataListWrapper). Alternatively we could do all of this in the DataListWrapper.

Yup, just me not being Reactified enough - changed to setState with a complete new DataListWrapper.

I think passing the filter down to the cells moves logic that the filter cares about to elements that shouldn't have to be aware of that. The cells should only care about displaying the data, i.e. they need to be able to use whatever the getObjectAt throws at them but filtering doesn't feel like anything that they should care about.

Agreed we should not pass down the filters, just a filtered data list

…d non-reacty wrapper behavior

Re-added pending-cell component

wcjordan · 2016-11-17T16:09:45Z

examples/CtxtFilterExample.js

+    }
+  }
+
+  filter() {


This looks better, thanks! I think we can go a bit further cleaning it up, but I'll wait until we have this using context since I think that will present it's own set of challenges and I don't want to get distracted.

Created an advanced example

Added comments and cleaned up code base

gforge · 2016-11-18T15:47:42Z

This message was created automatically by mail delivery software.

A message that you sent could not be delivered to one or more of its
recipients. This is a temporary error. The following address(es) deferred:

dr.max.gordon@gmail.com
Domain gforge.se has exceeded the max emails per hour (11/10 (110%)) allowed. Message will be reattempted later

------- This is a copy of the message, including all the headers. ------
Received: from github-smtp2-ext5.iad.github.net ([192.30.252.196]:47696 helo=github-smtp2a-ext-cp1-prd.iad.github.net)
by srv02.cpanel.pin.se with esmtps (TLSv1:DHE-RSA-AES256-SHA:256)
(Exim 4.87)
(envelope-from noreply@github.com)
id 1c7O3b-0007X8-3Z
for max@gforge.se; Thu, 17 Nov 2016 15:52:55 +0100
Date: Thu, 17 Nov 2016 06:52:13 -0800
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com;
s=pf2014; t=1479394333;
bh=6f9HOaPKffga62ygBFo9gDo49emlj/L9YN0Zia4SLfw=;
h=From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID:
List-Archive:List-Post:List-Unsubscribe:From;
b=QkdM/17u7pIqdNvdSZv1kJV/5vh2Inb5vm6jbvREIS3x+nx4UNflHLnY4bvHRtIsj
55DpWrCI3nBNcKNlQCUXgeOwczdEg0eOuEaXby3sJJIY2YNwKYIINuHIGzOfV7ildv
oDjlAeLiYOYlTnbsOdn7jfu0759itkeMfPDFkvA8=
From: Chris Jordan notifications@github.com
Reply-To: schrodinger/fixed-data-table-2 reply@reply.github.com
To: schrodinger/fixed-data-table-2 fixed-data-table-2@noreply.github.com
Cc: Max Gordon max@gforge.se,
Author author@noreply.github.com
Message-ID: schrodinger/fixed-data-table-2/pull/80/review/9039637@github.com
In-Reply-To: schrodinger/fixed-data-table-2/pull/80@github.com
References: schrodinger/fixed-data-table-2/pull/80@github.com
Subject: Re: [schrodinger/fixed-data-table-2] Adding examples using context
(#80)
Mime-Version: 1.0
Content-Type: multipart/alternative;
boundary="--==_mimepart_582dc41d8367d_4b193fd87acc7134521d0";
charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: list
X-GitHub-Sender: wcjordan
X-GitHub-Recipient: gforge
X-GitHub-Reason: author
List-ID: schrodinger/fixed-data-table-2
<fixed-data-table-2.schrodinger.github.com>
List-Archive: https://github.com/schrodinger/fixed-data-table-2
List-Post: mailto:reply@reply.github.com
List-Unsubscribe: mailto:unsub+00218d81a7663a5f428a2ff8ad97fd73252750cc287ce89a92cf000000011445861d92a169ce0b5346d3@reply.github.com,
https://github.com/notifications/unsubscribe/ACGNgUcBWGNRf8SRVeodNiqbVVtx9jlgks5q_GodgaJpZM4K1IlE
X-Auto-Response-Suppress: All
X-GitHub-Recipient-Address: max@gforge.se

----==_mimepart_582dc41d8367d_4b193fd87acc7134521d0
Content-Type: text/plain;
charset=UTF-8
Content-Transfer-Encoding: 7bit

wcjordan commented on this pull request.

if (Object.keys(filters).length > 0) {
```
 const filteredIndexes = [];
```

 for (let index = 0; index < this.state.rawData.getSize(); index += 1) {

   const row = this.state.rawData.getObjectAt(index);

   // Loop through all the filters and check if there's a match

```
   const found = Object.keys(filters)
```
```
     .map((varName) => {
```
```
       const value = row[varName];
```

       // If we have a set of values e.g. an array

       //  If you're using immutablejs then you can just use List

       //  This can be useful when you have more complex matches

```
       if (value instanceof Array) {
```

         const matches = value.map(x => match(x, filters[varName])).filter(x => x === true);

         // If all filters were identified then set this to true

ya, just meant the comment is misleading / should be updated to reflect that

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#80
----==_mimepart_582dc41d8367d_4b193fd87acc7134521d0
Content-Type: text/html;
charset=UTF-8
Content-Transfer-Encoding: 7bit

@wcjordan commented on this pull request.

In examples/CtxtFilterExample.js:

> +    if (Object.keys(filters).length > 0) {
+      const filteredIndexes = [];
+      for (let index = 0; index < this.state.rawData.getSize(); index += 1) {
+        const row = this.state.rawData.getObjectAt(index);
+
+        // Loop through all the filters and check if there's a match
+        const found = Object.keys(filters)
+          .map((varName) => {
+            const value = row[varName];
+
+            // If we have a set of values e.g. an array
+            //  If you're using immutablejs then you can just use List
+            //  This can be useful when you have more complex matches
+            if (value instanceof Array) {
+              const matches = value.map(x => match(x, filters[varName])).filter(x => x === true);
+              // If all filters were identified then set this to true

ya, just meant the comment is misleading / should be updated to reflect that

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

----==_mimepart_582dc41d8367d_4b193fd87acc7134521d0--

Added except to clear propTypes from being propagated

…airbnb style guide

ContextExample touch ups

gforge added 14 commits November 10, 2016 23:02

Converted pagination example to use context

6452c35

Externalized the data from the HOC

4800fc4

Removed the cryptic "end"-state and _dataVersion Changd _updateData() to refresh() to better represent the function's purpose

Removed the Pendingcell as it wasn't needed

f425f57

Moved HOC into HOC helpers and made the component more extendeable by…

59b526b

… adding a custom PropType that only checks for the presence of the setCallback function

Cleaned up the HOC design

40b8080

Moved data into a prop and created a HOCed DataTable that is used in the actual example

WIP: started working on the filtertable

123d39b

WIP: cleaned the filter and started working out the issues

2a6a6b3

TODO: FilterTable should probably be a HOC as well

Add Cell import

535df12

WIP: Filter example runs but doesn't filter :-(

fc25d22

Added back to old Filter/Pag. examples

076bd30

Moved context to Ctxt....

Added new context pages

53d7818

Added a constant value 'fileName' without the EXAMPLES_LOCATION_BASE The file is autogenerated from the EXAMPLES_LOCATION_BASE

Renamed context cell file

75e5a64

Fixed the filtering example

420021e

First functioning 'context' as suggested in issue schrodinger#76

401b6f8

wcjordan reviewed Nov 17, 2016

View reviewed changes

gforge added 5 commits November 17, 2016 15:52

Removed setIndexMap in filter example

e3569b7

Changed map over filters into for-loop

c93774d

Fixed according to comments regarding clumsy filter implementation an…

3d42e07

…d non-reacty wrapper behavior

Added version to data HOC

072aa88

Re-added pending-cell component

Merge branch 'context'

71efde7

wcjordan reviewed Nov 17, 2016

View reviewed changes

gforge added 5 commits November 17, 2016 23:24

Moved all HOC into helpers/HOC

c8ed4a6

Created an advanced example

Merge branch 'context'

b0e75d7

Removed the versioning and callback handling since it didn't help

d9c8063

Fixed a working sorted, filtered, pagintated example with context

be4b90a

Added comments and cleaned up code base

Merge branch 'context'

394a254

gforge and others added 8 commits November 19, 2016 11:46

Fixed code to match most of the airbnb eslint style guides

c6cecb1

Added except to clear propTypes from being propagated

Style updates

fe96503

Removed the purt filter and pagination examples

54d5a84

Removed sort from example and fixed a few styles to better match the …

099f6eb

…airbnb style guide

ContextExample touch ups

14f06e2

missed a spot

c90811e

Merge pull request #1 from wcjordan/context_example

1589c24

ContextExample touch ups

Merge branch 'master' into master

c401252

wcjordan merged commit 4eb5019 into schrodinger:master Nov 28, 2016

wcjordan mentioned this pull request Nov 28, 2016

Pull filtering into data list store for context example #86

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding examples using context #80

Adding examples using context #80

gforge commented Nov 17, 2016

wcjordan Nov 17, 2016

gforge Nov 17, 2016

wcjordan Nov 17, 2016

gforge Nov 17, 2016

wcjordan Nov 17, 2016 •

edited

Loading

gforge Nov 17, 2016

wcjordan Nov 17, 2016

gforge Nov 18, 2016

wcjordan Nov 17, 2016

gforge Nov 17, 2016

wcjordan Nov 17, 2016

gforge Nov 17, 2016

wcjordan Nov 17, 2016

gforge Nov 17, 2016

wcjordan Nov 17, 2016

gforge Nov 17, 2016

wcjordan Nov 17, 2016 •

edited

Loading

gforge Nov 17, 2016

wcjordan Nov 17, 2016

gforge Nov 17, 2016

wcjordan Nov 17, 2016

gforge Nov 17, 2016

wcjordan Nov 17, 2016

wcjordan Nov 17, 2016

gforge commented Nov 18, 2016

Adding examples using context #80

Adding examples using context #80

Conversation

gforge commented Nov 17, 2016

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

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

wcjordan Nov 17, 2016 • 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

Choose a reason for hiding this comment

wcjordan Nov 17, 2016 • 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

gforge commented Nov 18, 2016

wcjordan Nov 17, 2016 •

edited

Loading

wcjordan Nov 17, 2016 •

edited

Loading