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

Add custom TextBadge() component #459

Merged
merged 11 commits into from
Jun 9, 2023

Conversation

mxmvncnt
Copy link
Contributor

@mxmvncnt mxmvncnt commented Jun 8, 2023

This PR is not done yet, I want to make it so that it is possible to edit the value in the app settings.

  • Change Badge component to Box (allows to change the shape)
  • Set a more squared-off hard-coded value
  • Change the text colour to OnTertiary (avoids contrast issues)
  • Create a separate TextBadge component
  • Change the radius value in the settings

Addresses #92

This allows to customize the shape. I also changed the color of the text to OnTertiary to avoid contrast issues.
@mxmvncnt mxmvncnt requested a review from dessalines as a code owner June 8, 2023 02:06
@mxmvncnt
Copy link
Contributor Author

mxmvncnt commented Jun 8, 2023

Here is a quick before/after of the changes. I had not realized that the colour was actually fine before. Oh well...

244243998-5108dc38-1417-45ce-9230-f67f6bdc7cbd-obfuscated
244243999-7dda613a-ae75-462a-9c63-10b81e62c070-obfuscated

@TrollBlox
Copy link

Maybe it's just me, but the text looks a little lower inside the flair.

@mxmvncnt
Copy link
Contributor Author

mxmvncnt commented Jun 8, 2023

Maybe it's just me, but the text looks a little lower inside the flair.

I think this is the default behaviour for fonts, they are "optically" centered so that when there is a longer capital letter it looks fine. In my screenshots it is all lower case so maybe that's why. But I will check with the padding as maybe I made a mistake or should adjust some things

@kuro-codes
Copy link
Contributor

If you want to fix your checks run./gradlew formatKotlin

@dessalines dessalines marked this pull request as draft June 8, 2023 18:46
@dessalines
Copy link
Member

Thanks! Mark this as "ready to review" when you're ready.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

There are also two other badges, use the find in files and search for Badge(

@@ -34,14 +39,19 @@ fun PersonName(
val style = MaterialTheme.typography.bodyMedium

if (isPostCreator) {
Badge(
containerColor = MaterialTheme.colorScheme.tertiary,
Box(
Copy link
Member

Choose a reason for hiding this comment

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

Might as well externalize this to its own composable, we'll probably have more uses for it later on. Put it in one of the files (or a new one), in ui/common/...

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 will do just that, thanks!

Copy link
Contributor Author

@mxmvncnt mxmvncnt Jun 8, 2023

Choose a reason for hiding this comment

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

@dessalines Hey, I didnt change the other Badge() since it looks like they are being used as their intended use case, which is to show a number for like notifications. And I am not sure if this badge should be affected by the corner radius changes since they are very small and I don't think there would be a way to make them slightly squared off but still looking nice, I feel the radius would make it look off-circley, kind of like a broken circle.

Copy link
Member

Choose a reason for hiding this comment

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

Mmmk that's fine.

name = "op_comment_badge_radius",
defaultValue = DEFAULT_OP_COMMENT_BADGE_RADIUS.toString(),
)
val opCommentBadgeRadius: Int
Copy link
Member

Choose a reason for hiding this comment

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

This is way overkill, no need for any custom setting here. Lets just have this be a sane default. Literally every tiny thing could be tweaked, from icon sizes, to spacing of every little thing. This one doesn't need it.

Copy link
Contributor Author

@mxmvncnt mxmvncnt Jun 9, 2023

Choose a reason for hiding this comment

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

If no custom settings are needed then I guess its ready for review/merge !

Copy link
Contributor Author

@mxmvncnt mxmvncnt Jun 9, 2023

Choose a reason for hiding this comment

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

ugh, just noticed the conflict

@mxmvncnt mxmvncnt marked this pull request as ready for review June 9, 2023 00:12
@mxmvncnt mxmvncnt requested a review from twizmwazin as a code owner June 9, 2023 00:12
Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

LGTM

@dessalines
Copy link
Member

dessalines commented Jun 9, 2023

btw @twizmwazin does it let you click the enable auto-merge button? Whoever is the 2nd reviewer should feel free to do that.

@twizmwazin twizmwazin enabled auto-merge (squash) June 9, 2023 04:28
@twizmwazin twizmwazin merged commit 1362432 into LemmyNet:main Jun 9, 2023
@twizmwazin twizmwazin mentioned this pull request Jun 9, 2023
@mxmvncnt mxmvncnt changed the title Change badge component from Badge to Box Add custom TextBadge() component Jun 13, 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.

5 participants