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

[docs] Improve EnhancedTable demo #19363

Closed
MareoRaft opened this issue Jan 23, 2020 · 10 comments · Fixed by #19560
Closed

[docs] Improve EnhancedTable demo #19363

MareoRaft opened this issue Jan 23, 2020 · 10 comments · Fixed by #19560
Assignees
Labels
docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@MareoRaft
Copy link

I would suggest for the sorting examples some name changes that make it clearer what is going on.

cmp -> comparator or cmp

desc -> descendingComparator or descCmp

getSorting -> getComparator or getCmp

K -> Key? or KeyType? I'm not sure what this should be called because I haven't learned what "key in K" means.

Thanks!

@MareoRaft
Copy link
Author

I would also suggest moving the "getSorting" function definition (and the Order type) immediately below the "desc" function definition, since this would put "desc", "getSorting", and "stableSort" into logical order.

@eps1lon
Copy link
Member

eps1lon commented Jan 23, 2020

Sounds good. Would you like to work on a PR?

@eps1lon eps1lon added docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. labels Jan 23, 2020
@MareoRaft
Copy link
Author

Well, are there other sorting examples in the repo other than the one I found? It seems to me that the changes are trivial but perhaps somebody more familiar with the repo and its conventions would be the one to actually do it. But I would be willing to do it with some guidance.

@eps1lon
Copy link
Member

eps1lon commented Jan 23, 2020

Well, are there other sorting examples in the repo other than the one I found?

One at a time.

It seems to me that the changes are trivial but perhaps somebody more familiar with the repo

There are plenty of trivial changes but only a limited number of contributors. Every contribution is appreciated. Especially for getting to know the codebase these little changes to readability are very nice.

For an initial primer I recommend starting with https://github.com/mui-org/material-ui/blob/master/CONTRIBUTING.md. You do not need to read everything but the basics for setting up the repo and making your first PR.

@oliviertassinari oliviertassinari changed the title better sorting variable names [docs] Improve EnhancedTable demo Jan 27, 2020
@oliviertassinari
Copy link
Member

@MareoRaft Do you want to move forward with the changes and submit a pull request? :)

@TommyJackson85
Copy link
Contributor

@MareoRaft Do you want to move forward with the changes and submit a pull request? :)

@MareoRaft I am happy to try this myself instead if you're working on something else. Up to you.

@ahmad-reza619
Copy link
Contributor

Hi everyone, i would like to take this one on, but is there anyone have taken this? if not i'll try Proposing PR tonight. BTW i'm a new in terms of contributing so any guidance will be appreciated

@eps1lon
Copy link
Member

eps1lon commented Feb 4, 2020

Hi everyone, i would like to take this one on, but is there anyone have taken this? if not i'll try Proposing PR tonight. BTW i'm a new in terms of contributing so any guidance will be appreciated

@ahmad-reza619 I think you're good to go since we haven't heard anything from @TommyJackson85.

https://github.com/mui-org/material-ui/blob/master/CONTRIBUTING.md should explain most of the workflow. You can still open a PR if you're stuck and need more help to get the PR in a mergable state.

@eps1lon eps1lon assigned eps1lon and ahmad-reza619 and unassigned eps1lon Feb 4, 2020
@TommyJackson85
Copy link
Contributor

TommyJackson85 commented Feb 4, 2020

@eps1lon I was waiting for a confirmation on if I can go ahead, like being assigned, because @MareoRaft already agreed to do it over 12 days ago. :)

@ahmad-reza619 sure go ahead if you haven't contributed yet!

@MareoRaft
Copy link
Author

@ahmad-reza619 All yours. Thanks for your help. I'm going to stop watching this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants