Skip to content

New assist "Wrap with Expanded/Flexible" #56648

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

Closed
FMorschel opened this issue Sep 4, 2024 · 9 comments
Closed

New assist "Wrap with Expanded/Flexible" #56648

FMorschel opened this issue Sep 4, 2024 · 9 comments
Labels
devexp-assist Issues with analysis server assists legacy-area-analyzer Use area-devexp instead. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@FMorschel
Copy link
Contributor

I'd like to request a new kind of "Wrap with Widget" assist.

This would probably need to check for the parent to see if it is a Flex but since some of the other assists like "remove widget" already test for multiple children, I think this might be fine.

I am requesting this because I use this a lot in my trees and I believe it would be a simple addition with great use (even Widget Inspector shows this as an important factor in layout). I'm not sure if this has been discussed before but I could not find any issues here.

@dart-github-bot
Copy link
Collaborator

Summary: The user proposes a new "Wrap with Expanded/Flexible" assist that would automatically wrap selected widgets with Expanded or Flexible based on the parent widget being a Flex. This would simplify layout adjustments and improve developer workflow.

@dart-github-bot dart-github-bot added legacy-area-analyzer Use area-devexp instead. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-enhancement A request for a change that isn't a bug labels Sep 4, 2024
@bwilkerson bwilkerson added P3 A lower priority bug or feature request devexp-assist Issues with analysis server assists labels Sep 4, 2024
@FMorschel
Copy link
Contributor Author

I've noticed:

When coming from #55825. Just to point out they exist in this issue since they seem related.

@lrhn lrhn removed the triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. label Sep 6, 2024
@FMorschel
Copy link
Contributor Author

@tenhobi
Copy link
Contributor

tenhobi commented Oct 18, 2024

@FMorschel can I ask you why do you allow wrapping with Expanded when parent is not a widget, and not just for Flex parents?
image

@tenhobi
Copy link
Contributor

tenhobi commented Oct 18, 2024

Do you plan to add assist to add a Flexible as well, as suggested in issue description?

@FMorschel
Copy link
Contributor Author

@FMorschel can I ask you why do you allow wrapping with Expanded when parent is not a widget, and not just for Flex parents?

I've copied the base tests from Center and reshaped them for Expanded. I've also added tests for Flex, Row and Column and made sure that Stack is one other children example that doesn't trigger.

The reason why I added that second part is because without it, 8 of the 15 tests break because these fundaments change:

  • Being the first widget inside a method/function that returns Widget (both BlockBody and ExpressionBody)
  • Assignment to a Widget variable

Someone might want to save their Expanded widget stored inside another class declaration for future use (assignment) or to make an ExpandedFoo which will be used inside a Flex widget later.

@FMorschel
Copy link
Contributor Author

Do you plan to add assist to add a Flexible as well, as suggested in issue description?

I was thinking about it but I thought about making one work and get approved might be better because then adding the second one is easier and because I'm not sure we'd like to fill the Wrap with list with too much options. So I intend on asking a team member about this.

@tenhobi
Copy link
Contributor

tenhobi commented Oct 19, 2024

@FMorschel regarding the Expanded assist on widget without parent, I really think that in practice it's always safer and more clear to do:

Column(
  children: [
    Expanded(
      child: MyCustomWidget(),
    ),
  ],
)

vs what you are talking about and what current solution promotes/allows:

Column(
  children: [
    MyCustomWidget(), // with Expanded inside as first widget
  ],
)

Therefore me personally I don't see a good reason why to suggest Expanded/Flexible on widgets without parent, since it can introduce and promote bugs, it's potentially not safe, and therefore me personally I would only want this assist when parent is Column/Row. And if the developer thinks they know what they are doing or whatever, they can still use "Wrap with ..." or write it manually. I think assists should aim to be as safe as possible.

@FMorschel
Copy link
Contributor Author

FMorschel commented Oct 19, 2024

I see your concern and I think it is valid to think this way.

In this case I'm pressing more on consistency here. People will wonder and ask for things like:

Widget build(BuildContext context) {
var myChild = ColoredBox(color: Colors.red);
var myChildren = [
  ColoredBox(color: Colors.red),
];
return Column(
  children: [
    ...myChildren,
    myChild,
  ],
}

Why can't I add Expanded around either ColoredBox in this case? They will be used inside the Flex widget. Or even cases like the following where the person knows what it is doing, it would not be possible to add the Expanded with the assist because the previous widget is not a Flex.

Column(
  children: [
    Builder(
      builder: (context) {
        return Expanded( // <- Add this, you can copy to see the code works just fine
          child: Container(color: Colors.red),
        );
      },
    ),
  ],
)

So I think it is easier for us to ask the people to "use it carefully" than to remove the option for them to use it only when it is 100% safe. If they are using the assist, they will assist but they'll not do the job for you.

And like I said, if you still have a tree like:

Container(
  child: Container(
  //...
  ),
);

Or with Stack or whatever multi child option, in this case the inner Container won't show the option to wrap it with Expanded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-assist Issues with analysis server assists legacy-area-analyzer Use area-devexp instead. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants