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.

Sign up for GitHub

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

Jump to bottom

Fix of "Title wraps within word in Improve the Experience screen" #8322

Merged
merged 4 commits into from
Oct 21, 2024
Merged

Fix of "Title wraps within word in Improve the Experience screen" #8322

merged 4 commits into from
Oct 21, 2024

Conversation

shamim-emon
Copy link
Contributor

@shamim-emon shamim-emon commented Oct 13, 2024 •
edited
Loading

  • In Improve the Experience screen, made title auto-resize so it always fits in the available width

Fixes #8257

@shamim-emon shamim-emon changed the title Fix of "Title wraps within word in "Improve the Experience" screen" Fix of "Title wraps within word in Improve the Experience screen" Oct 13, 2024
@shamim-emon
Copy link
Contributor Author

@cketti @wmontwe please let me know if I've missed anything. Thanks!

@cketti
Copy link
Member

cketti commented Oct 15, 2024 •
edited
Loading

Thanks for your pull request 👍

We have talked about this internally and decided on a path forward. Please see #8257 (comment)

I've created PR #8341 to switch to showing the brand name. Once that is merged it would be awesome if you could look into resizing the text so it fits the available width.

Side note: Please don't merge main into your pull request branch. If you want/need to update the pull request branch, rebase it on top of the updated main branch instead.

@shamim-emon
Copy link
Contributor Author

Thanks for your pull request 👍

We have talked about this internally and decided on a path forward. Please see #8257 (comment)

I've created PR #8341 to switch to showing the brand name. Once that is merged it would be awesome if you could look into resizing the text so it fits the available width.

Side note: Please don't merge main into your pull request branch. If you want/need to update the pull request branch, rebase it on top of the updated main branch instead.

@cketti I have made corresponding text auto resizable so it always fits the available width. Please let me know if that's alright and/or if anything else needed.

@shamim-emon shamim-emon force-pushed the fix-issue-8257 branch 3 times, most recently from cdccd7c to c8b8519 Compare October 18, 2024 17:24
@shamim-emon
Copy link
Contributor Author

@cketti @wmontwe I was wondering if you guys were able to take a look? Did I miss something / anything else needed?
Please let me know if anything more needs to be done from my end.

cketti
cketti requested changes Oct 21, 2024
View reviewed changes
...ystem/src/main/kotlin/app/k9mail/core/ui/compose/designsystem/atom/text/TextDisplayMedium.kt Outdated
Comment on lines 75 to 79
if (style.fontSize.isUnspecified) {
resizedTextStyle = resizedTextStyle.copy(
fontSize = defaultFontSize,
)
}
Copy link
Member

Choose a reason for hiding this comment

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

All of our text styles contain a font size. So this code is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

...ystem/src/main/kotlin/app/k9mail/core/ui/compose/designsystem/atom/text/TextDisplayMedium.kt Outdated
@@ -39,3 +46,43 @@ fun TextDisplayMedium(
style = MainTheme.typography.displayMedium,
)
}

@Composable
fun AutoResizeableTextDisplayMedium(
Copy link
Member

Choose a reason for hiding this comment

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

Please rename to TextDisplayMediumAutoResize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

...ystem/src/main/kotlin/app/k9mail/core/ui/compose/designsystem/atom/text/TextDisplayMedium.kt Outdated
modifier: Modifier = Modifier,
color: Color = Color.Unspecified,
textAlign: TextAlign? = null,
style: TextStyle = MainTheme.typography.displayMedium,
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the style parameter.

If we need this functionality for another text style we can then figure out how best to generalize (and where to move) this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@shamim-emon
Copy link
Contributor Author

@cketti @wmontwe all reviews are resolved + All checks have passed. If possible, please have a look and let me know if anything else required.
Thanks in advance.

@cketti cketti merged commit 450d8f1 into thunderbird:main Oct 21, 2024
2 checks passed
@cketti
Copy link
Member

cketti commented Oct 21, 2024

Thanks 👍

@shamim-emon shamim-emon deleted the fix-issue-8257 branch October 21, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cketti cketti cketti approved these changes

@wmontwe wmontwe Awaiting requested review from wmontwe wmontwe is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Title wraps within word in "Improve the Experience" screen
2 participants
@shamim-emon @cketti