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

Visual test #10

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Visual test #10

wants to merge 8 commits into from

Conversation

BreimerR
Copy link

image

@DevSrSouza
Copy link
Owner

Hi @BreimerR, could you explain your use case? I did a test here and seems that Compose default Modifier.size() has the same effect side by side.

I think your implementation is perfect to work with applying a base size to the SVG like 32dp is by default, but I think that the name in the Icon should be optional.

image

@BreimerR
Copy link
Author

BreimerR commented Mar 26, 2022

Referenced from: Boiler code reduction.
Instead of using Modifier.size(). The commit resizes all the nodes. Thus no need for the Modifier.

@Composable
fun Icon(){
     Icon(Icons.SomeLargeIcon,modifier = Modifier.size(24.dp))
}

Becomes

@Composable
fun SomeLargeIcon(){
     Icon(Icons.SomeLargeIcon24)
}

@DevSrSouza
Copy link
Owner

I don't think this is a good solution for removing boilerplate, Icons generation generation alot alot alot of code generation that can ingress code size to just not use Modifier.size().
Compose is flexible, you can always create a custom icon function that was size pre defined. I will protype a function for that use case: something like AwesomeIcons.Something.size64

@DevSrSouza
Copy link
Owner

DevSrSouza commented Mar 26, 2022

I still think your PR is awesome but generating multiple icons with difference only on the icon size is not a good a idea in my opinion, but allowing to generate a icon with a specified size would be awesome and this PR also support that.
If you are able to open a new PR adding the scale/size functionality would be awesome.

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.

2 participants