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

feat(compose) Add Grapes Tag #278

Merged
merged 5 commits into from
Oct 20, 2023
Merged

feat(compose) Add Grapes Tag #278

merged 5 commits into from
Oct 20, 2023

Conversation

jpbarbaud
Copy link
Contributor

Add new Grapes Tap component

Preview

Type Screenshot
Error Screenshot 2023-10-18 at 16 17 52
Warning Screenshot 2023-10-18 at 16 17 56
Info Screenshot 2023-10-18 at 16 18 00
Success Screenshot 2023-10-18 at 16 18 03

Usage

Error

GrapesErrorTag(
    label = "Label",
    showIcon = true // Optional - true by default
)

Warning

GrapesWarningTag(
    label = "Label",
    showIcon = true // Optional - true by default
)

Info

GrapesInfoTag(
    label = "Label",
    showIcon = true // Optional - true by default
)

Success

GrapesSuccessTag(
    label = "Label",
    showIcon = true // Optional - true by default
)

if (icon != null) {
Box(
modifier = Modifier
.size(GrapesTagDefaults.iconSize)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this does not define the size of the icon but the size of the container of the icon. Do you really need it?
If somewhere you put GrapesTagIcon(iconRes = R.drawable.ic_error, contentDescription = "Error tag icon", modifier = Modifier.size(64.dp)) what happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I put the image inside a sized box, the image will automatically fit inside this box and it allows me to center the image inside this box.
Moreover, nothing prevents us from having another component than Icon in the slot, the box gives us more control over the content.

The box acts like a container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-10-18 at 17 27 58

Copy link
Collaborator

Choose a reason for hiding this comment

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

The fact that the icons fit is because the design is done well no? If you manually set the size of the icon bigger, it will act weirdly i guess.
I guess the main question is :
What's the point of having that box?

  • If it's only to center the icon, you can remove it and use a verticalArrangement on the row fixes your screen.
    ( btw it's in fact mandatory to make sure the text is centered well in the tag with any font zoom)
    Here everything fits because it has been done pixel perfect on figma
  • If it's to enforce an icon size, it won't work, it will just enforce that the icon size do not make the height of the tag grow. But if the size defined in the icon is bigger than the size of the box it will be cropped (and then we have to decidewhat is better. a big tag with nothing cropped or a tag the regular size with the icon cropped)
    And if the size defined in the icon is smaller, it will be centered inside the box and then the paddings between the icon and the text won't be the same as for other tags. Because the padding is defined between the label and the box.

Comment on lines +27 to +29
containerColor: Color = GrapesTheme.colors.mainAlertLightest,
contentColor: Color = LocalContentColor.current,
borderStoreColor: Color = GrapesTheme.colors.mainAlertLighter,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel it weird to use both mecanism at the same time. To me it should be either of them.
You can provide a DefaultGrapesTagColors to GrapesTag and internally it's GrapesTag that do the CompositionLocalProvider( LocalContentColor provides GrapesTheme.colors.mainAlertNormal, )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to take advantage of the LocalContentColor.
I was inspired by the Material TextFieldDefaults.textFieldColors().

Copy link
Collaborator

Choose a reason for hiding this comment

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

Check how the button is done. It's in fact the surface that defines the LocalContentColor so we should even have to add it ourselves

@jpbarbaud jpbarbaud merged commit 6a94eb4 into develop Oct 20, 2023
3 checks passed
@jpbarbaud jpbarbaud deleted the task/tag_component branch October 20, 2023 07:59
@jpbarbaud jpbarbaud mentioned this pull request Oct 20, 2023
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