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] Don't dispatch ignored "reset code variant" actions #27712

Merged
merged 1 commit into from Aug 14, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Aug 12, 2021

We default to the previous codeVariant anyway if we receive a falsy codeVariant. Might as well not dispatch it at all.

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Aug 12, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Aug 12, 2021

No bundle size changes (experimental)

Generated by 🚫 dangerJS against 6d21025

@@ -226,7 +226,7 @@ export default function DemoToolbar(props) {
};

const handleCodeLanguageClick = (event, clickedCodeVariant) => {
if (codeVariant !== clickedCodeVariant) {
if (clickedCodeVariant !== null && codeVariant !== clickedCodeVariant) {
Copy link
Member

Choose a reason for hiding this comment

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

Off-topic

Should ToggleButtonGroup enforce to retain the active value when set to exclusive? This demo feels weird https://next.material-ui.com/components/toggle-button/#exclusive-selection, what does no text alignment mean? 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

Should ToggleButtonGroup enforce to retain the active value when set to exclusive?

I think the confusion comes from the ambiguity of exclusive. With your proposal exactly one value would have to be selected. The current implementation allows 0 or 1 values to be selected. In other words: With your proposal we could not de-select.

But this doesn't block this PR so I'm just assuming this means approval. Please approve explicitly in the future and generally be more specific in your reviews. They require almost always another round-trip that could've been avoided by explicit messaging.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, agree. I guess if this exclusive pain comes again on the GitHub issues, we could continue the conversion further.

But this doesn't block this PR

@eps1lon You are right.

We were facing a similar pain in the X repo. A couple of weeks ago, in the retrospective, we settled on using Off-topic when we wanted to start discussions on parallel problems that aren't relevant for merging the PR. Sometimes, it's about potentially going after the root cause, others about weird things in the code that was already present, or else.

Please approve explicitly in the future

I'm aiming to approve and request changes on as few PRs as possible so that the maintainers can step in, making myself redundant.

I think that it's also a way for the other members to build up confidence in taking on larger problems. I know that you usually have strong points of view, and a long history contributing to the codebase, but it's not necessarily the case for everybody.

It's also about recognizing that things might not be done as I think should be, so to trust the person who takes the call, and make her/him accountable for the implications.

@eps1lon eps1lon merged commit 4d4e4f3 into mui:next Aug 14, 2021
@eps1lon eps1lon deleted the docs/language-reset branch August 14, 2021 08:44
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants