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.

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

adding docs: Actions, Bulk Selection, Types; expose ActionTypes #136

Merged
merged 3 commits into from
Mar 14, 2017

Conversation

headwinds
Copy link
Contributor

I've started three new docs for Actions, Bulk Selection and Types

Similar to applyGridConfig which has all the Grid Types, I've exposed the ActionTypes but used the simple * approach to pull in everything so now all the types are available and can be listened to from reducers outside of the react-redux-grid library.

import { applyGridConfig } from './constants/GridConstants';
import * as ActionTypes from './constants/ActionTypes';

const modules = {
    Actions,
    Grid,
    GridRootReducer: combineReducers(Reducers),
    Reducers,
    applyGridConfig,
    ActionTypes,
    Store
};

@bencripps
Copy link
Owner

This is so awesome, thanks for the work! I will take a good look at all of this tonight, and leave comments if I have any.

Copy link
Owner

@bencripps bencripps left a comment

Choose a reason for hiding this comment

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

Thanks for doing all the hard work! Just a couple of comments for things I'd like to see resolved before the merge.

@@ -0,0 +1,65 @@
## Actions

Each of the reducers: grid, pager, selection, etc have supporting actions. You may wish to dispatch these actions from other areas of your application outside of the grid. For instance, you could unselect all the rows of the grid that has checkbox column.
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe "For instance, you could select all/deselect all rows in a grid from another component in your application"? is a better use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's better

Use them in your components from this.props

```
uncheckAllSelectedRows(event){
Copy link
Owner

Choose a reason for hiding this comment

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

Let's remove everything from Use them in your components from this.props down, in this file. Once merged, I'll go back and provide the use case for how best to use the actions.

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'll simply this section to:

Use them in your components from this.props.

  // get the all selection row ids
  const gridMap = this.props.selection.get("myGridStateKey");
  const selectedIds =  gridMap.get("indexes");
  
  // call a bulk selection action to deselect all the rows  
  const stateKey = "myGridStateKey";
  this.props.deselectAll(stateKey); 

#### grid vs dataSource vs selection reducers

const columnData = this.props.grid.get('sticky');
console.log(JSON.stringify(columnData) ); <-- only column data
Copy link
Owner

Choose a reason for hiding this comment

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

Since you can console.log complex objects, we probably don't need the JSON.stringify here.

Copy link
Contributor Author

@headwinds headwinds Mar 13, 2017

Choose a reason for hiding this comment

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

Can you really?! How do you console log an ordered map to see it's data structure?! amazing if you can...

Articles like this suggest its not possible

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, if it's an ordered map, then let's do console.log(columnData.toJS());




```
Copy link
Owner

Choose a reason for hiding this comment

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

Cant we update the alignment here, or is this just a github issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep done

const columnIndex = 0;
updateTabIndex(columnIndex)

#### Disabling Tab Index
Copy link
Owner

Choose a reason for hiding this comment

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

Let's remove this part too, we can do this in a more reduxy way. I will update after the merge,

Copy link
Contributor Author

@headwinds headwinds Mar 13, 2017

Choose a reason for hiding this comment

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

As for this, what I really wanted was to be able to configure the column and pass in tabIndex prop for each one. If I want to disable it, I could pass "-1"

I'll remove my hack ;-D


You have may other components that may wish to listen to these commands and you can use the React-Redux-Grid types in your own reducers.

#### Grid Types
Copy link
Owner

Choose a reason for hiding this comment

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

What are Grid Types -- aren't these the same as Action Types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh ok... I saw different types within it... there are GirdConstants.js and ActionTypes.js

GridConstants are already exposed as part of applyGridConfig

@@ -8,13 +8,15 @@ import { Reducers } from './reducers';
import { Actions } from './actions';

import { applyGridConfig } from './constants/GridConstants';
import * as ActionTypes from './constants/ActionTypes';
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind removing rootReducer as one of the exports? It was added a loooong time ago, and it's no longer necessary. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean delete these lines?

import { Reducers } from './reducers';
GridRootReducer: combineReducers(Reducers),

?

Copy link
Owner

Choose a reason for hiding this comment

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

Just delete the GridRootReducer: combineReducers(Reducers), since we still need to export the Reducers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I delete that line, and run the tests...they fail

@bencripps
Copy link
Owner

Your latest commit looks good, except there's a bunch of merge conflicts :( which is why the tests are all failing.

@headwinds
Copy link
Contributor Author

Ok I did do a pull with the upstream master before and found 1 conflict within dataSource which I resolved...

I get a green light on "This branch has no conflicts with the base branch" but that's probably my base master not upstream master...

@headwinds
Copy link
Contributor Author

headwinds commented Mar 13, 2017

Why won't it list which files are conflicting now?!

@headwinds
Copy link
Contributor Author

Ok I see dataSource.js is still messed - I'll fix the conflicts

@bencripps
Copy link
Owner

Since you didn't change dataSource, you should probably just checkout the current version of the file from bencripps/master

@codecov-io
Copy link

codecov-io commented Mar 13, 2017

Codecov Report

Merging #136 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
+ Coverage   81.52%   81.53%   +0.01%     
==========================================
  Files         112      112              
  Lines        4524     4527       +3     
==========================================
+ Hits         3688     3691       +3     
  Misses        836      836
Impacted Files Coverage Δ
src/index.js 94.44% <100%> (+1.11%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc5039b...ee36d75. Read the comment docs.

@headwinds
Copy link
Contributor Author

So now I just need to squash these comments down ;-D

@bencripps
Copy link
Owner

No need to squash. I'll take a look, and let you know if any other changes are needed. Thanks!

@bencripps
Copy link
Owner

Did we lose some of the changes when you pushed the new code? It looks like we lost some, or am I going crazy?

@headwinds
Copy link
Contributor Author

Nope not crazy - there should have been 5 not 4 files changed - I'm reviewing...I'll see dropped

@headwinds
Copy link
Contributor Author

Make that 6 files - I ran the tests and discovered that exposing AcitonTypes also requires a couple tests

@bencripps bencripps merged commit 2a9bdca into bencripps:master Mar 14, 2017
bencripps added a commit that referenced this pull request Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants