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

[ButtonBase] Allow to customize the link component via theme #25331

Merged
merged 13 commits into from
Mar 18, 2021

Conversation

vedadeepta
Copy link
Contributor

Fixes: #21533

@mui-pr-bot
Copy link

mui-pr-bot commented Mar 13, 2021

Details of bundle changes

Generated by 🚫 dangerJS against de527e2

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Why did you modify the source of the Button? (Why is ButtonBase not enough?)

@vedadeepta
Copy link
Contributor Author

vedadeepta commented Mar 13, 2021

You're right, Button modification is not needed. I will make changes

@vedadeepta
Copy link
Contributor Author

vedadeepta commented Mar 13, 2021

@oliviertassinari

Well, now that I think about it modifying Button is necessary to support this kind of theme config as discussed in the issue

const theme = createMuiTheme({
  props: {
    MuiLink: {
      component: RouterLink, 
    },
    MuiButton: {
     link: RouterLink, // 👈 explicit option for "links-as-buttons" 
    }
  },
});

Since we want the link option to be inside MuiButton not inside MuiButtonBase.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 13, 2021

Since we want the link option to be inside MuiButton not inside MuiButtonBase.

Why? This sounds counter-intuitive to me. Isn't the objective to replace all the links?

@oliviertassinari oliviertassinari added the new feature New feature or request label Mar 13, 2021
@oliviertassinari oliviertassinari changed the title [Button, ButtonBase]: add ability to customize link component via theme [Button, ButtonBase] Allow to customize the link component via theme Mar 13, 2021
@vedadeepta
Copy link
Contributor Author

vedadeepta commented Mar 13, 2021

No particular reason, I was following this
comment in the issue, seems like its how they want it. But I can change the API such that link is is inside MuiButtonBase of the theme if you think that makes more sense. I don't have strong preference either way.

objective to replace all the links?

Both ways accomplishes this objective. Right? There is a single place to configure the Link component used by buttons.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 13, 2021

@vedadeepta Yes, I think that we should ignore Button because it's only one of the wrappers built on top of ButtonBase. It's only relevant as a use case. Props are also spreading so, whatever ButtonBase supports, Button does to.

@vedadeepta
Copy link
Contributor Author

makes sense, will fix it tomorrow.

@oliviertassinari oliviertassinari changed the title [Button, ButtonBase] Allow to customize the link component via theme [ButtonBase] Allow to customize the link component via theme Mar 13, 2021
@oliviertassinari oliviertassinari added the component: ButtonBase The React component. label Mar 14, 2021
@oliviertassinari oliviertassinari force-pushed the 21533-Link-config branch 2 times, most recently from 3178de5 to d37d676 Compare March 14, 2021 17:06
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I have tried to improve the docs by creating a new section only for Routing. It seems to be a common pain point.
https://deploy-preview-25331--material-ui.netlify.app/guides/routing/

@vedadeepta vedadeepta closed this Mar 14, 2021
@vedadeepta vedadeepta reopened this Mar 14, 2021
@vedadeepta
Copy link
Contributor Author

any update on this PR?

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 18, 2021

@vedadeepta I wanted to make a small change to the markdown before merging. To expose the theme structure without requiring developers to click anything.

@vedadeepta
Copy link
Contributor Author

aight, makes sense

@oliviertassinari oliviertassinari merged commit fca2ddf into mui:next Mar 18, 2021
@oliviertassinari
Copy link
Member

@vedadeepta Done, thanks for dedicating care and time to this :)

@vedadeepta
Copy link
Contributor Author

thanks for reviewing & improving the PR, appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ButtonBase The React component. new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure "Link" component via Theme
3 participants