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-13311 Evolve Feedback Screen #300

Merged
merged 6 commits into from
Sep 21, 2023

Conversation

jeslat
Copy link
Contributor

@jeslat jeslat commented Sep 7, 2023

🥅 What's the goal?

Evolve Feedback Screen:

  • Reduce side margins from 24px to 16px
  • Decrease icon size from 64 to 48
  • Custom success animation for Vivo_New
  • Update cascading opacity effect for screen content

🚧 How do we do it?

  • First commit: remove duplicated resources (they were the same the common and the Telefonica style).
  • Second commit: modify margins of the layout
  • Third commit: update success animation for Vivo new
  • Fourth commit: update the entrance animation

☑️ Checks

  • I updated the documentation, including readmes and wikis. If this is a breaking change, update UPGRADING.md to inform users how to proceed. If no updates are necessary, indicate so.
  • Tested with dark mode.
  • Tested with API 23.

🧪 How can I test this?

  • You can use the generated catalog to see all the different combinations due to in some cases, the icon/animation changes per brand.

  • Video with the feedbacks in Vivo-New and the new animations:

feedback-vivonew.mp4

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

📱 New catalog for testing generated: Download

@Telefonica Telefonica deleted a comment from github-actions bot Sep 7, 2023
@jeslat jeslat marked this pull request as ready for review September 11, 2023 10:48
@jeslat jeslat requested review from a team, dpastor, jdelga, jeprubio and AnaMontes11 and removed request for a team and dpastor September 11, 2023 10:49
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.

This seem changes in the classic views, should it be also done in compose? Or am I missing something?

@jeslat
Copy link
Contributor Author

jeslat commented Sep 11, 2023

This seem changes in the classic views, should it be also done in compose? Or am I missing something?

@jeprubio the compose implementation is just a wrapper of the XML view so no need to change anything

@github-actions
Copy link

📱 New catalog for testing generated: Download

Copy link
Member

@jdelga jdelga left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@AnaMontes11 AnaMontes11 left a comment

Choose a reason for hiding this comment

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

We have informed Vivo about the internal margin of the icon, which causes it to appear misaligned with the text and smaller in size. Additionally, there is an issue with the size of the Lottie animation that was detected by @jeslat. Vivo has informed us that they are preparing the correct assets. We are still waiting for them to provide us with these assets so that we can include them.

You can provide more context here: https://jira.tid.es/browse/OBVIVO-1792

android:layout_marginTop="24dp"
android:scaleType="center"
app:layout_constraintStart_toStartOf="parent"
android:layout_height="48dp"

Choose a reason for hiding this comment

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

I understand that the icon is at 48, and that's correct, but even with the internal margin we discussed earlier, it still appears visually too small. Is it possible for the icon to scale according to the screen density?
61112349-2b1d-45fc-a3ed-385c4bd43600

Copy link
Contributor

@jeprubio jeprubio Sep 14, 2023

Choose a reason for hiding this comment

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

That 48dp, the dp in there means density-independent pixels so it's scaling according to screen density. I suspect the feedback_success_vivo_new.json has some margins in it so then when the height are set to 48dp it has to be shrunk to fit in there, I would check that.

@github-actions
Copy link

📱 New catalog for testing generated: Download

@yamal-alm
Copy link
Contributor

📱 New catalog for testing generated: Download

Hi @AnaMontes11 This new catalog includes the new lottie asset for Vivo New. How about now? 🙂

Captura de Pantalla 2023-09-21 a las 9 32 53

Copy link

@AnaMontes11 AnaMontes11 left a comment

Choose a reason for hiding this comment

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

Now looks nice!

@yamal-alm yamal-alm merged commit b23fdee into main Sep 21, 2023
4 checks passed
@yamal-alm yamal-alm deleted the task/ANDROID-13311-feedbackscreen branch September 21, 2023 13:47
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