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.

Sign up for GitHub

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

Jump to bottom

Added rowStyle prop to SimpleList #5252

Merged
merged 8 commits into from
Oct 19, 2020
Merged

Added rowStyle prop to SimpleList #5252

merged 8 commits into from
Oct 19, 2020

Conversation

ValentinnDimitroff
Copy link
Contributor

@ValentinnDimitroff ValentinnDimitroff commented Sep 14, 2020 •
edited by fzaninotto
Loading

Closes #5253

@ValentinnDimitroff ValentinnDimitroff mentioned this pull request Sep 14, 2020
Closed
jdemangeon
jdemangeon suggested changes Sep 14, 2020
View reviewed changes
Copy link
Contributor

@jdemangeon jdemangeon 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 your PR! Can you rebase on next and make test pass please?

packages/ra-ui-materialui/src/list/SimpleList.tsx Outdated Show resolved Hide resolved
@djhi
Copy link
Collaborator

djhi commented Sep 18, 2020

Hey, thanks for the pull request. Would you mind to also run the prettier command (yarn prettier) to fix the CI build?

@ValentinnDimitroff ValentinnDimitroff mentioned this pull request Sep 25, 2020
Closed
@ValentinnDimitroff
Copy link
Contributor Author

I have ran the yarn prettier and made a new PR to the next branch.

djhi
djhi requested changes Oct 7, 2020
View reviewed changes
Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Can you please revert the two unrelated changes (cypress.json and package.json)?

@ValentinnDimitroff
Copy link
Contributor Author

Ok, sorry guys for the other PR, I got little bit confused with you asked me to do but I think now it should be ok?

I rebased to the next branch. Still got CI issues, I don't get it why.

@djhi Yes, I will revert those changes. I think they came after I ran the yarn prettier.

@djhi
Copy link
Collaborator

djhi commented Oct 7, 2020

Yes, I will revert those changes. I think they came after I ran the yarn prettier.

Ah sorry about that, I'll take a loot at its configuration. Thanks for reporting it!

@ValentinnDimitroff
Copy link
Contributor Author

It's fine, I am a newly contributor so still getting to grips. If some other changes are needed just ping me and I will update the branch! :)

@djhi
Copy link
Collaborator

djhi commented Oct 7, 2020

Ignore my comments on the unrelated files. It is indeed prettier related and it's actually right.

djhi
djhi requested changes Oct 7, 2020
View reviewed changes
packages/ra-ui-materialui/src/list/SimpleList.tsx Outdated
<ListItem
button={!!linkType as any}
style={
rowStyle ? rowStyle(data[id], rowIndex) : null
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems you didn't replace null with undefined as requested by @jdemangeon. Can you please fix that?

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 have but accidentially reverted this one also. Brought it back again, sorry.

@djhi djhi added this to the 3.9.3 milestone Oct 7, 2020
djhi
djhi approved these changes Oct 7, 2020
View reviewed changes
Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Thanks!

@djhi
Copy link
Collaborator

djhi commented Oct 8, 2020

Sorry again, would you mind documenting the feature as well ? The file to update is https://github.com/marmelab/react-admin/blob/master/docs/List.md#the-simplelist-component

@ValentinnDimitroff
Copy link
Contributor Author

Sorry again, would you mind documenting the feature as well ? The file to update is https://github.com/marmelab/react-admin/blob/master/docs/List.md#the-simplelist-component

I was actually already doing this :) No worries - will update in a while.

@fzaninotto fzaninotto removed this from the 3.9.3 milestone Oct 8, 2020
@fzaninotto
Copy link
Member

You said you rebased your PR on next, yet you opened it against master. Can you change the PR destination?

@ValentinnDimitroff ValentinnDimitroff changed the base branch from master to next October 8, 2020 13:08
@ValentinnDimitroff
Copy link
Contributor Author

You said you rebased your PR on next, yet you opened it against master. Can you change the PR destination?

Thanks for the patience - finally did it the right way I hope!

@fzaninotto
Copy link
Member

Yep, the PR is on the right branch now. We're just waiting for your documentation update.

@ValentinnDimitroff
Copy link
Contributor Author

I added just a new row in the table with props as it is well described in the Datagrid section. Is it needed to provide again a usage example?

@djhi
Copy link
Collaborator

djhi commented Oct 9, 2020 •
edited
Loading

Is it needed to provide again a usage example?

It would be way better 👍 Thanks for taking the time to complete this!

@ValentinnDimitroff
Copy link
Contributor Author

Sorry for the late reply - I have added the usage of the prop with editing the existing example in order to keep the page tight and clean. Also tried to stick to the Datagrid example reference. If anything is missing just comment and I'll polish it till the end :) (responding quite faster from now on)

djhi
djhi approved these changes Oct 19, 2020
View reviewed changes
Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

👍 Thanks a lot!

@djhi djhi added this to the 3.10.0 milestone Oct 19, 2020
@djhi djhi merged commit aa639d9 into marmelab:next Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdemangeon jdemangeon jdemangeon requested changes

@djhi djhi djhi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.10.0
Development

Successfully merging this pull request may close these issues.

Make SimpleList compatible with rowStyle property.
4 participants
@ValentinnDimitroff @djhi @fzaninotto @jdemangeon