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

ANDROID-15405 -- PosterCard component #398

Merged
merged 40 commits into from
Dec 3, 2024

Conversation

dhonti-axpe
Copy link
Contributor

@dhonti-axpe dhonti-axpe commented Nov 21, 2024

🥅 What's the goal?

First approach about PosterCard component creation according to current Card Spec section.

Links references:

🚧 How do we do it?

  • First of all, a PosterCard.kt file exposes the main component through is provided corresponding implementation.
  • Corresponding section in library catalog README.md
  • This component is divided in 3 sections:
    • TopSection: Possibility to add a maximum of 2 TopAction actionable rounded (or not) icons
    • MainSection
      • Tag
      • Pretitle
      • Title
      • Subtitle
      • Description
    • CustomSection: A custom composable that will be integrated at bottom of current component

Warning

This implementation does not include the background video

☑️ Checks

  • I updated the documentation, including readmes and wikis. If this is a breaking change, tag the PR with "Breaking Change" label and remember to include breaking change migration guide in release notes where this version is released.
  • Tested with dark mode.
  • Tested with API 24.
  • Sync done with iOS team for this feature to ensure alignment, if applies.
  • Accessibility considerations.

🧪 How can I test this?

  • 🖼️ Screenshots/Videos
Main functionality A11 integrated
postercard_sample.webm
postercard_talkback.webm
  • Mistica App QR or download link
  • Reviewed by Mistica design team

Copy link

📱 New catalog for testing generated: Download

Copy link

📱 New catalog for testing generated: Download

Copy link

📱 New catalog for testing generated: Download

- Added param in "TopAction" component to be able to receive a "testTag".
- FIXED problem resize Image()
Copy link

📱 New catalog for testing generated: Download

…xed aspect ratio and the height inner content is bigger than aspect ratio height.
Copy link

📱 New catalog for testing generated: Download

Copy link

📱 New catalog for testing generated: Download

Copy link

📱 New catalog for testing generated: Download

@dhonti-axpe dhonti-axpe changed the title ANDROID-15405 - PosterCard component ANDROID-15405 -- PosterCard component Nov 25, 2024
Copy link

📱 New catalog for testing generated: Download

Copy link

📱 New catalog for testing generated: Download

Copy link
Contributor

@dpastor dpastor left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@haynlo haynlo left a comment

Choose a reason for hiding this comment

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

Waiting for a11y implementation for final review.

Copy link

📱 New catalog for testing generated: Download

Copy link
Contributor

@yceballost yceballost left a comment

Choose a reason for hiding this comment

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

nice job Ɛ>

Copy link

📱 New catalog for testing generated: Download

Copy link

📱 New catalog for testing generated: Download

Copy link

github-actions bot commented Dec 2, 2024

📱 New catalog for testing generated: Download

@dhonti-axpe dhonti-axpe requested a review from haynlo December 2, 2024 15:14
) {
with(topActionData) {
Box(
modifier = Modifier
Copy link
Contributor

@jeprubio jeprubio Dec 2, 2024

Choose a reason for hiding this comment

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

I think this should be modifier = modifier and not the parameter applied in the then() at the end. If you want to add some external paddings, alignment to a box... to the TopAction it won't allow you this way it's done in here if I'm not wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're so right!

verticalAlignment = Alignment.CenterVertically
) {
firstTopAction?.let {
TopAction(topActionData = it, modifier = Modifier.semantics { traversalIndex = 7f })
Copy link
Contributor

@jeprubio jeprubio Dec 2, 2024

Choose a reason for hiding this comment

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

Does traversalIndex work without defining a traversalGroup? I can't find a traversalGroup in the pr and my experience was that it doesn't work, idk.

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 forgot remove that reference to semantics in this composable. Change in next commit

content: @Composable BoxScope.() -> Unit
) {
Box(
modifier = Modifier
Copy link
Contributor

@jeprubio jeprubio Dec 2, 2024

Choose a reason for hiding this comment

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

Here also I think the modifier parameter should be the first modifier to apply and not the last one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, at first, we opted this approach using semantics() previously. Since that if you want to apply semantics() attributes is necessary do it finally in your modifier. But now this is not necessary. I'll change that!


@Composable
fun PosterCard(
modifier: Modifier = Modifier,
Copy link
Contributor

Choose a reason for hiding this comment

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

https://blog.simprasuite.com/jetpack-compose-respect-the-contract-of-modifiers-ecbbe8ce03db#:~:text=The%20modifier%20should%20be%20the,other%20parameters%20with%20default%20values.

The modifier should be the first optional parameter in the parameter list; after all required parameters (except for trailing lambda parameters like card content) but before any other parameters with default values.

@dhonti-axpe dhonti-axpe requested a review from jeprubio December 2, 2024 16:05
Copy link

github-actions bot commented Dec 2, 2024

📱 New catalog for testing generated: Download

Copy link
Contributor

@jeprubio jeprubio left a comment

Choose a reason for hiding this comment

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

👍

…postercard

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
Copy link

github-actions bot commented Dec 3, 2024

📱 New catalog for testing generated: Download

@dhonti-axpe dhonti-axpe merged commit 36a19aa into main Dec 3, 2024
5 checks passed
@dhonti-axpe dhonti-axpe deleted the dhonti/ANDROID-15405-postercard branch December 3, 2024 15:02
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