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

Demo app cart #518

Merged
merged 7 commits into from
Aug 9, 2024
Merged

Demo app cart #518

merged 7 commits into from
Aug 9, 2024

Conversation

magda-woj
Copy link
Contributor

Adding product of specified quantity to cart and emptying cart works. QuantityChooser component moved to a separate file.

@magda-woj magda-woj requested a review from a team August 8, 2024 18:25
Comment on lines 91 to 111
DropdownMenuItem(
text = {
Row(
verticalAlignment = Alignment.CenterVertically
) {
if (i == selectedQuantity) {
Icon(
imageVector = Icons.Filled.Check,
contentDescription = "Selected",
tint = Color.Black,
modifier = Modifier.size(18.dp)
)
Spacer(modifier = Modifier.width(8.dp))
}
Text(
text = i.toString(),
)
}
},
onClick = { onQuantitySelected(i) }
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that this was broken out into its own file. I think @LikeTheSalad had a comment previously about indent levels, and there's still some room to improve this. If you make a new topmost item called something like QuantityDropDownItem (whose constructor takes i and a boolean isSelected) then you can basically move this whole block into that new composable and save 1 or 2 indent levels. The for loop then looks something like

for (i in 1..10) {
    QuantityDropDownItem(i, i == selectedQuantity)
}

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

One small recommendation about reducing indent, but otherwise looking good to me. Thanks for the help!

… added to cart - range from 1 to 10, like on the otel demo astronomy shop website.
…it should be in the center?), split up QuantityChooser into a few composables.
…n CartScreen makes the screen respond to Empty Cart button right away and i don't know how to achieve this otherwise
Copy link
Contributor

@LikeTheSalad LikeTheSalad left a comment

Choose a reason for hiding this comment

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

Thanks!

@breedx-splk breedx-splk merged commit 063acd9 into open-telemetry:main Aug 9, 2024
3 checks passed
@magda-woj magda-woj deleted the cart branch August 21, 2024 09:09
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.

3 participants