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

Add name option to makeStyles #3946

Merged
merged 3 commits into from
Nov 26, 2019

Conversation

DanudeSandstorm
Copy link
Contributor

I've begun adding the name of the component in the ra-ui-materialui components that use makeStyles as mentioned in #3701 .
Please let me know whether this is what is expected and I can perform this addition on the remaining components.

Note: There's a difference in prettier/linting specification between .tsx and .js files, which results in differences in the makeStyles formatting.

@DanudeSandstorm DanudeSandstorm changed the title Add name option to makeStyles [WIP] [WIP] Add name option to makeStyles Nov 7, 2019
Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

Yep, that's definitely going in the right direction!

packages/ra-ui-materialui/src/auth/Logout.tsx Outdated Show resolved Hide resolved
@DanudeSandstorm DanudeSandstorm force-pushed the makestyles-options branch 2 times, most recently from 8496ef7 to 90b97bc Compare November 7, 2019 22:00
@DanudeSandstorm DanudeSandstorm changed the title [WIP] Add name option to makeStyles Add name option to makeStyles Nov 7, 2019
Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

I think we should prefix all names with 'Ra' - just like material-ui prefixes all their names with 'Mui'. That's because the names go to a pool shared between libraries, and there is a good chance that List and other generic names are already present.

I'm sorry to realize that just now, after telling you to go ahead with the initial design, but I just understood the risk by reading your PR.

Would you mind updating all the names you've added to add the 'Ra' prefix?

packages/ra-ui-materialui/src/detail/TabbedShowLayout.js Outdated Show resolved Hide resolved
@WiXSL
Copy link
Contributor

WiXSL commented Nov 25, 2019

@DanudeSandstorm can you rebase?

@DanudeSandstorm
Copy link
Contributor Author

I rebased onto next

@WiXSL
Copy link
Contributor

WiXSL commented Nov 25, 2019

Thank you.
Cause I need those changes for styling as well.

@WiXSL
Copy link
Contributor

WiXSL commented Nov 26, 2019

@fzaninotto , is this PR ready to be merged?

@fzaninotto fzaninotto merged commit 5c38c08 into marmelab:next Nov 26, 2019
@fzaninotto
Copy link
Member

Excellent, thanks!

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