Skip to content

Fix: Reduce logo size and ensure it resizes dynamically and fits all screen sizes #8710

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
Merged
merged 6 commits into from
Mar 17, 2025
Merged

Fix: Reduce logo size and ensure it resizes dynamically and fits all screen sizes #8710

merged 6 commits into from
Mar 17, 2025

Conversation

SttApollo
Copy link
Contributor

Fixes #8458

Description

This PR addresses #8458 where the logo on the onboarding screen flattened the text below. The WelcomeLogo composable has been updated to ensure dynamic resizing using BoxWithConstraints ensuring that the logo scales proportionally based on screen dimensions and fits properly without overlapping or pushing other components off-screen.

Changes Made

  • Updated WelcomeLogo to use BoxWithConstraints for adaptive sizing.
  • Limited logo and circle sizes to a maximum based on screen width.
  • Ensured LazyColumnWithHeaderFooter components are unaffected by the logo's size.
  • Verified compatibility across various screen sizes (small, medium, large, and tablets).

Testing

  • Tested on multiple screen sizes to ensure no scrolling is required and all components fit properly.

Visual Example

Screenshot 2024-12-29 at 3 33 02 PM
Screenshot 2024-12-29 at 3 28 56 PM
Screenshot 2024-12-29 at 3 29 19 PM

@wmontwe wmontwe self-assigned this Jan 6, 2025
wmontwe
wmontwe reviewed Jan 9, 2025
View reviewed changes
Copy link
Member

@wmontwe wmontwe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@SttApollo This implementation reduces the logo size across all screen sizes, which diverges from the original design intention of making it prominent. To fix this, I suggest two changes:

  1. Slighlty reduce the logo size from its original values to have more room in general.
  2. Only use the shrinking logic when a certain maxWidth threshold is reached.

(Optional) Given the impracticality of fitting all page elements on every screen size, especially very small phones and in landscape mode, I think it would be more effective to use a scroll indicator in these cases.

@SttApollo SttApollo requested a review from kewisch as a code owner February 6, 2025 21:04
@SttApollo
Copy link
Contributor Author

SttApollo commented Feb 6, 2025 •
edited
Loading

Hii @wmontwe - thank you for your patience. I slightly reduced the logo size and added a custom scroll indicator ( I couldn't find it in the project files and wasn't sure if I could add additional libraries).

Below are additional notes, brief screen recordings and screenshots to explain this implementation. Please let me know your thoughts and I'll adjust accordingly again, as needed.

  • Custom Scroll indicator caches item heights using onGloballyPositioned for accurate scroll behavior
  • Inserted a spacer at index=3, ensuring small phones register sufficient overflow for the scrollbar logic to prevent situations where the footer is just slightly cut off from the screen (noticed that issue when testing on smaller devices)
  • Maintained vertical space evenly design
    Screenshot 2025-02-06 at 3 33 52 PM
    Screenshot 2025-02-06 at 11 05 44 AM
    SmallPhonee
    Pixel3.webm
    Pixel9ProXL.webm

@SttApollo
Copy link
Contributor Author

Hi @wmontwe! I was trying to make sure the commit passes detekt, so I moved the scroll indicator function to a different file and shortened the welcome content. Do you want me to keep working on these changes or revert to the last commit state?

@wmontwe
Copy link
Member

wmontwe commented Feb 19, 2025

@SttApollo I rebased the pull-request on the latest version of main.

I tried the solution, but I see some performance issues with the scroll indicator that causes compose to redraw a lot.

I checked if there is another solution available, but not really. Worst case without the indicator, but I would like to look into it a bit.

@SttApollo
Copy link
Contributor Author

@SttApollo I rebased the pull-request on the latest version of main.

I tried the solution, but I see some performance issues with the scroll indicator that causes compose to redraw a lot.

I checked if there is another solution available, but not really. Worst case without the indicator, but I would like to look into it a bit.

Sounds good - I'll hold until further. Perhaps, I can look at other bugs I can be assigned to.

@kewisch
Copy link
Member

kewisch commented Mar 6, 2025

@wmontwe any updates on this? I know you've been out a bit, when you have a chance to get back to it I'd appreciate!

@wmontwe wmontwe removed the request for review from cketti March 6, 2025 17:09
@wmontwe
Copy link
Member

wmontwe commented Mar 6, 2025

@SttApollo Unfortunately, my attempts to enhance the scroll indicator were unsuccessful. It seems this requires significant efforts and should be a separate task.

I think it's best to remove it for now and I have a look at the version without scroll indicator.

Sounds good - I'll hold until further. Perhaps, I can look at other bugs I can be assigned to.

Is there a specific area of interest (UI etc)?

@SttApollo
Copy link
Contributor Author

I have removed the scroll indicator @wmontwe. And yes the UI for my first few issues! Is there anything in #8812 epic I may be able to support? Also open to other suggestions you may have!

@kewisch kewisch requested a review from wmontwe March 10, 2025 16:29
wmontwe
wmontwe requested changes Mar 12, 2025
View reviewed changes
Copy link
Member

@wmontwe wmontwe left a comment

Choose a reason for hiding this comment

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

I think the size changes are working very good. Just the logo and title should be part of the header section. The Box around the layout is not needed, as it prevents the list to draw it's content to full screen size.

You could fix the formatting issues by running ./gradlew spotlessApply. Run ./gradlew detekt to find other issues.

...nboarding/welcome/src/main/kotlin/app/k9mail/feature/onboarding/welcome/ui/WelcomeContent.kt Outdated
@@ -1,5 +1,6 @@
package app.k9mail.feature.onboarding.welcome.ui


Copy link
Member

Choose a reason for hiding this comment

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

I guess your using a different code format. Just run ./gradlew spotlessApply to streamline your changes with our code format. If you update the branch to current main, Spotless is also way quicker.

SttApollo and others added 2 commits March 15, 2025 13:08
@SttApollo @wmontwe
Apply suggestions from code review
824f67b 
Co-authored-by: Wolf-Martell Montwé <hi@wolfmontwe.com>
@SttApollo
Fix WelcomeHeaderSection and apply Spotless
ef7fcae
@SttApollo
Copy link
Contributor Author

@wmontwe thank you, done!

wmontwe
wmontwe approved these changes Mar 17, 2025
View reviewed changes
Copy link
Member

@wmontwe wmontwe left a comment

Choose a reason for hiding this comment

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

Thank you!

@wmontwe wmontwe merged commit db176ec into thunderbird:main Mar 17, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wmontwe wmontwe wmontwe approved these changes

@kewisch kewisch Awaiting requested review from kewisch

Assignees

@wmontwe wmontwe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Welcome / Get started screen has vertical content overflow
3 participants
@SttApollo @wmontwe @kewisch