Skip to content

Home Screen - #61

Open
Maria1-eng wants to merge 1 commit into
mainfrom
homescreen-design
Open

Home Screen#61
Maria1-eng wants to merge 1 commit into
mainfrom
homescreen-design

Conversation

@Maria1-eng

Copy link
Copy Markdown
Collaborator

This commit implements a preview design, updates the strings according to the Figma design, and adds the background image.

This commit implements a preview design, updates the strings according to the Figma design, and adds the background image.
@Maria1-eng
Maria1-eng requested a review from Tyler-Lopez May 10, 2026 21:55
Comment on lines +72 to +79
.padding(bottom = 180.dp),
)

if (showCreateActivityButton) {
Column(
modifier = Modifier
.align(Alignment.BottomCenter)
.padding(horizontal = 24.dp, vertical = 48.dp)
.fillMaxWidth(),

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I like how this looks, but I feel the paddings look poorly implemented. How could I improve the spaces ? I thought about changing the text to inside column.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I thought about changing the text to inside column

Is this referring to "Track" and "Improve"? I agree, I think a Column is a really good idea :)

In the future, the "Connect with Strava" and "Create Strava Activity" buttons won't be here. Those were just temporary. I think it makes the most sense for those to be in the post-workout screen.

@Tyler-Lopez Tyler-Lopez left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks great :) nice job! Feel free to merge whenever you want. If you would like to change anything, I left some comments but don't feel like you need to change anything.

fontFamily = rockSalt,
fontSize = 65.sp,
color = Color.Black,
lineHeight = 52.sp,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Rather than specifying the lineHeight and using a single String like "Track\nImprove" in a single Text, I might suggest that you instead put this in a Column of two Text. What do you think? This is totally fine though!

)

Text(
text = "Track\nImprove",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We should also probably translate this :) so that means making a new String resource in those files, one for English, one for Spanish - then replacing this "hard coded String" with that.

Comment on lines +72 to +79
.padding(bottom = 180.dp),
)

if (showCreateActivityButton) {
Column(
modifier = Modifier
.align(Alignment.BottomCenter)
.padding(horizontal = 24.dp, vertical = 48.dp)
.fillMaxWidth(),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I thought about changing the text to inside column

Is this referring to "Track" and "Improve"? I agree, I think a Column is a really good idea :)

In the future, the "Connect with Strava" and "Create Strava Activity" buttons won't be here. Those were just temporary. I think it makes the most sense for those to be in the post-workout screen.

Comment on lines +86 to +87
.fillMaxWidth()
.height(56.dp),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
.fillMaxWidth()
.height(56.dp),
.fillMaxWidth(),
contentPadding = PaddingValues(
horizontal = 24.dp,
vertical = 12.dp,
),

We should get rid of this. In its place, we can increase the padding :)

Why? We should do this because font-sizes are not always the same, users can increase the font size of their device. Being honest with you, most applications don't really care about this 😆 even really big applications just totally suck at big font sizes.

Here, I increase the font size of my device.

With a big font size, sometimes the button needs to be more than 56.dp in height. Using contentPadding makes sure everything is more consistent. :) You can see how in the "with height" Column the text gets cut-off from "Create Strava Activity".

With height With contentPadding
Image Image

By the way, we can also prevent text from scaling, you normally should avoid that, but we can do it for "Track" and "Improve" to avoid what you see here :)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We actually have this problem so much worse in other parts of the application 😆 the big font-sizes breaking the application, so definitely no huge deal here

contentScale = ContentScale.FillHeight,
modifier = Modifier
.padding(20.dp)
.height(48.dp)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This one is ok to have height specified, because it is an image, which does not scale with the user's settings.

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