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

End of Year: Design tweaks (part ii) #619

Merged
merged 18 commits into from
Dec 1, 2022

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Nov 30, 2022

📘 Project: #410

Description

Applies small design tweaks:

Intro Screen

  • Moved of to the second line to avoid widow in listening... ("en" lang only)
  • Used an exclamation point instead of ellipses
  • The 2022 logo moved downwards so it’s optically better centered

Generic

  • Replaced apostrophe in Let's with a typographic one
  • Moved the Pocket Casts logo downwards, so there isn’t such a big empty space between it and the Share button
  • Covers made bigger ListenedCategories, LongestEpisode, TopPodcast and ListenedNumbers stories

Top Categories

  • Applied 3D effect to pillar
Top Categories

Epilogue

  • Added pulsating animation to the heart vector
  • Replaced Pocket Casts logo (with a red circle)

I also captured an UnknownFormatConversionException in 7.27 in e4fb26e. It is for the Arabic Letter Meem (م). The letter is used inside values-ar/strings.xml in several places. I'll get that fixed in a separate PR after consulting someone from the i18n team.

@ashiagr ashiagr requested a review from a team as a code owner November 30, 2022 08:19
@ashiagr ashiagr force-pushed the task/410-design-tweaks-partii branch from d32332e to e4fb26e Compare November 30, 2022 16:30
Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

I'll let @adamzelinski take a look at this from a UI perspective, but the code looks good.

The only thing I noticed that seemed off was that on the two screens where we removed the Share button, the content does not expand to take up the Share Button space. This is visible with a normal font size (because the content does not have a black background like the space where the share button does), but it is much more obvious when a larger font size is used where the user can scroll.

device-2022-11-30-131316.mp4

Also seems like the spacing gets rather crowded with the larger font size.

@@ -52,7 +59,7 @@ fun StoryEpilogueView(

Spacer(modifier = modifier.weight(1f))

HeartImage()
PulsatingHeart()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Copy link

@CookieyedCodes CookieyedCodes Nov 30, 2022

Choose a reason for hiding this comment

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

The only thing I think it needs matt is the last story if you try to skip the story it just loops back onto the last story, it should close the story 🤔 😉

(other then a story top 15-25 podcasts the current story should say add the time for the each of the top 5😉, theirs allways next year tho),
(last story should also collect all the story's and allows sharing of the 9 as a group as thats better for social engagement )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your feedback is valuable, @CookieyedCodes! Thanks for sharing it. 🙏 I'll add it to the backlog.

@ghost
Copy link

ghost commented Dec 1, 2022

Thanks for these updates @ashiagr it's looking great and I love the new heart animation! As for the text size on the screenshot, yes it looks funny if it scrolls. On iOS, we're using dynamic type but there is also a max size set so maybe we could look at doing something similar?

Also, in iOS on the first and last screen now that the share button is hidden we have expanded those stories
2022 Year In Podcasts 1
2022 Year In Podcasts 10

@ashiagr
Copy link
Contributor Author

ashiagr commented Dec 1, 2022

Thank you, for the feedback!

Also, in iOS on the first and last screen now that the share button is hidden we have expanded those stories

To match iOS, expanded those screens on Android. - eff6611

The only thing I noticed that seemed off was that on the two screens where we removed the Share button, the content does not expand to take up the Share Button space. This is visible with a normal font size (because the content does not have a black background like the space where the share button does), but it is much more obvious when a larger font size is used where the user can scroll.

On iOS, we're using dynamic type but there is also a max size set so maybe we could look at doing something similar?

To improve experience, disabled text scaling if the device font scale is beyond 1.15 (popular values: 1.0, 1.15, 1.3) - 1e2ed1f

Available font scales from Tracks:
Screenshot 2022-12-01 at 2 55 47 PM

Intro Epilogue
max_font_size.mp4

Feedback from Slack DM

  • Increase padding between artwork and text on ListenedNumbers story - a5a065c
Normal Scaled
  • Revert to the original logo on Epilogue screen - 71326bd

I'll address any feedback in a future PR.

@ashiagr ashiagr mentioned this pull request Dec 1, 2022
35 tasks
@ashiagr ashiagr merged commit face060 into release/7.27.1 Dec 1, 2022
@ashiagr ashiagr deleted the task/410-design-tweaks-partii branch December 1, 2022 09:56
@mchowning mchowning mentioned this pull request Dec 1, 2022
8 tasks
) {
Text(
text = text,
color = color,
fontSize = 22.sp,
lineHeight = 30.sp,
fontSize = if (disableScale) fontSize.value.nonScaledSp else fontSize,
Copy link
Contributor

Choose a reason for hiding this comment

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

One minor issue that I think this logic creates is that someone with a font scaled to 1.1 will have a bigger font than someone whose font is set to be 1.2 because the 1.2 gets reset to 1.0. Very minor issue, but I wanted to mention it.

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