Skip to content

Default sortMethod (plus: initialState from extra props)#718

Merged
ryanlanciaux merged 5 commits into
GriddleGriddle:masterfrom
NewBoCo:default-sortMethod
Aug 17, 2017
Merged

Default sortMethod (plus: initialState from extra props)#718
ryanlanciaux merged 5 commits into
GriddleGriddle:masterfrom
NewBoCo:default-sortMethod

Conversation

@dahlbyk

@dahlbyk dahlbyk commented Aug 14, 2017

Copy link
Copy Markdown
Contributor

Griddle major version

1.x

Changes proposed in this pull request

  • Type cleanup for RowRenderProperties and ColumnRenderProperties, which currently the same as RowDefinitionProps and ColumnDefinitionProps.
  • Rearranges how initialState is initialized to accept/override initialState from extra Griddle props. Everything still seems to work as expected, but please double-check this.
    • As a bonus, this allows setting enableSettings and textProperties without a plugin, as demonstrated with new stories.
  • Default sortMethod from state is used, if it exists.
  • Addressed TODO to get Next/Previous label from textProperties

Why these changes are made

<Griddle sortMethod={...}> now works as a high-level hook to override all columns' sort behavior. It does not change default behavior (cc #466).

Are there tests?

Stories!

@ryanlanciaux

Copy link
Copy Markdown
Member

Ahh shoot -- didn't mean to make conflicts here.

@dahlbyk

dahlbyk commented Aug 15, 2017

Copy link
Copy Markdown
Contributor Author

Ahh shoot -- didn't mean to make conflicts here.

It's fine, I knew it was coming.

@dahlbyk

dahlbyk commented Aug 15, 2017

Copy link
Copy Markdown
Contributor Author

(Conflicts have been resolved.)

Comment thread stories/index.tsx
})

.add('with custom default sort', () => {
return (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this have the sortProperties set like the previous story?

const sortProperties = [
  { id: 'name', sortAscending: true }
];
...
<Griddle
  ...
  sortProperties={sortProperties}
  sortMethod={sortBySecondCharacter}>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think this grid needs to be pre-sorted, no. This test is derived from the test that follows.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Got it - sorry about my confusion. 👍

@ryanlanciaux

Copy link
Copy Markdown
Member

This looks great! I've run this locally on some of my projects also and everything seems to work well. I just have the one question on the story but it looks good to merge.

@ryanlanciaux ryanlanciaux merged commit 4920b29 into GriddleGriddle:master Aug 17, 2017
@dahlbyk dahlbyk deleted the default-sortMethod branch August 17, 2017 12:46
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.

2 participants