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

Support Radio.adaptive for RadioMenuButton #147975

Open
huycozy opened this issue May 8, 2024 · 6 comments
Open

Support Radio.adaptive for RadioMenuButton #147975

huycozy opened this issue May 8, 2024 · 6 comments
Labels
c: proposal A detailed proposal for a change to Flutter f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. good first issue Relatively approachable for first-time contributors P2 Important issues not at the top of the work list team-design Owned by Design Languages team triaged-design Triaged by Design Languages team would be a good package Separate Flutter package should be made for this

Comments

@huycozy
Copy link
Member

huycozy commented May 8, 2024

Use case

RadioMenuButton uses Radio fixedly which supports Material Design only.

Proposal

Since we have Radio.adaptive, could we also have RadioMenuButton.adaptive?

Radio Radio.adaptive
@danagbemava-nc danagbemava-nc added in triage Presently being triaged by the triage team framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. c: proposal A detailed proposal for a change to Flutter team-design Owned by Design Languages team and removed in triage Presently being triaged by the triage team labels May 8, 2024
@Piinks Piinks added triaged-design Triaged by Design Languages team P2 Important issues not at the top of the work list labels May 8, 2024
@victorsanni
Copy link
Contributor

Same for CheckboxMenuButton:

@victorsanni victorsanni added good first issue Relatively approachable for first-time contributors labels Jun 4, 2024
@gerteck
Copy link

gerteck commented Jun 4, 2024

Could I give a try at contributing to this issue?

I would attempt to implement RadioMenuButton.adaptive following Radio.adaptive's implementation.

@victorsanni
Copy link
Contributor

Could I give a try at contributing to this issue?

I would attempt to implement RadioMenuButton.adaptive following Radio.adaptive's implementation.

A good first step would be defining a new RadioMenuButton.adaptive constructor. Then, in RadioMenuButton's build method, conditionally set the child property to either a Radio or Radio.adaptive depending on the type of the RadioMenuButton.

A similar pattern can be found here in CheckboxListTile using the _CheckboxType enum.

@gerteck
Copy link

gerteck commented Jun 4, 2024

A good first step would be defining a new RadioMenuButton.adaptive constructor. Then, in RadioMenuButton's build method, conditionally set the child property to either a Radio or Radio.adaptive depending on the type of the RadioMenuButton.

A similar pattern can be found here in CheckboxListTile using the _CheckboxType enum.

Got it! I've done so accordingly, I added both a _CheckboxType and a _RadioType enum, and conditionally set the child property accordingly. However, I didn't use a switch-case approach, but I'll modify it accordingly to match, and it does make the code easier to follow.

As per Tree-hygiene.md, I have yet to add any additional testcases for the changes made. I'm not entirely clear on how I should go about doing this. Should I:

  • Commit the changes I've made on my own branch, open a PR linking this issue, and work on testcases thereafter while seeking guidance if needed.
  • or
  • Work on the testcases first, and commit and open a PR only when I'm done.

Thank you for the previous reply! @victorsanni

@victorsanni
Copy link
Contributor

@gerteck both are valid.

@Piinks Piinks added the would be a good package Separate Flutter package should be made for this label Jun 12, 2024
@Piinks
Copy link
Contributor

Piinks commented Jun 12, 2024

From #149714, if we do implement a adaptive RadioMenuButton or CheckboxMenuButton, we should be able to strip way any material aspects like the ink splash when adapting to a cupertino look and feel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: proposal A detailed proposal for a change to Flutter f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. good first issue Relatively approachable for first-time contributors P2 Important issues not at the top of the work list team-design Owned by Design Languages team triaged-design Triaged by Design Languages team would be a good package Separate Flutter package should be made for this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants