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.

Sign up for GitHub

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

Jump to bottom

Update podcasts grid design #2165

Merged
merged 21 commits into from
May 9, 2024
Merged

Update podcasts grid design #2165

merged 21 commits into from
May 9, 2024

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented May 6, 2024 •
edited

Part of #2081

Description

This updates Podcasts tab grid design.
Figma Design: G60UqxPhTynbalo0Vfr1NC-fi-1255_8864

** I created a separate feature flag for these changes currently disabled in Firebase. It can be enabled in 7.64 if required.

Testing Instructions

  1. Open Podcasts tab
  2. Notice the grid spacing matches the design
  3. Tap more menu in the toolbar
  4. Select All unfinished for Badges
  5. ✅ Notice that a new unfinished badge is shown
  6. Select Only latest episode
  7. ✅ Notice that a new only latest badge is shown
  8. Switch to list view
  9. ✅ Notice that new badges(similar to iOS) are shown for the list view ** <- (Needs clarification G60UqxPhTynbalo0Vfr1NC-fi-1255_8864#793222421. I matched it with iOS.)
  10. Disable "Podcast grid view design changes" feature flag
  11. Kill and Restart the app
  12. ✅ Notice that original grid design changes are shown

Screenshots or Screencast

1

2

out-1

out-2

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes N/A
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml N/A
  • Any jetpack compose components I added or changed are covered by compose previews
  • I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics. N/A

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@ashiagr ashiagr added the Up Next label May 6, 2024
@ashiagr ashiagr added this to the Future milestone May 6, 2024
@ashiagr ashiagr requested a review from a team as a code owner May 6, 2024 11:27
@ashiagr ashiagr requested review from mebarbosa and removed request for a team May 6, 2024 11:27
@dangermattic
Copy link
Collaborator

dangermattic commented May 6, 2024 •
edited

1 Warning
⚠️ Class CountBadgeStyle is missing tests, but unit-tests-exemption label was set to ignore this.

Generated by 🚫 Danger

@ashiagr ashiagr mentioned this pull request May 6, 2024
Closed
mebarbosa
mebarbosa reviewed May 6, 2024
View reviewed changes
Copy link
Contributor

@mebarbosa mebarbosa left a comment

Choose a reason for hiding this comment

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

It looks good to me.

The only difference is the badge for only latest badge is shown if we compare with iOS, but since we are already waiting for clarification we can address this later

@mebarbosa mebarbosa self-requested a review May 6, 2024 13:11
@ashiagr
Copy link
Contributor Author

ashiagr commented May 8, 2024

I added a shadow for podcast grid items in f660207 (Ref: Automattic/pocket-casts-ios#1690 (comment)) and increased small badge size from 10dp to 12dp (Ref: Automattic/pocket-casts-ios#1689).

Shadow

Badge Size

Can you take another look @mebarbosa?

Base automatically changed from task/2081-revert-PRs to main May 8, 2024 12:31
@ashiagr
Merge branch 'main' into task/2081-podcasts-grid-design
dd66cbc 
# Conflicts:
#	CHANGELOG.md
#	modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/featureflag/Feature.kt
#	modules/services/views/src/main/res/values/dimens.xml
@ashiagr
Fix imports
a6f73f5
@ashiagr
Add click listener
b018bce
mebarbosa
mebarbosa reviewed May 8, 2024
View reviewed changes
...odcasts/src/main/java/au/com/shiftyjelly/pocketcasts/podcasts/view/podcasts/FolderAdapter.kt
val podcastBackground: View? = view.findViewById(R.id.header_background)
val podcastTitle: TextView = view.findViewById(R.id.library_podcast_title)
val author: TextView? = view.findViewById(R.id.podcast_author)
val unplayedText: TextView = view.findViewById(R.id.unplayed_count)
val unplayedBackground: ImageView? = view.findViewById(R.id.unplayed_background)
val countTextMarginSmall: Int = 2.dpToPx(view.resources.displayMetrics)
val cardElevation: Float = 2.dpToPx(view.resources.displayMetrics).toFloat()
val cardCornerRadius: Float = 4.dpToPx(view.resources.displayMetrics).toFloat()
Copy link
Contributor

Choose a reason for hiding this comment

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

Really small suggestion: What do you think we add a top padding for the badge?

I feel the new drop-shadow is clipping a bit the podcast above. See:

Screenshot_20240508_093732_Pocket Debug~2

and see the proposal here

@MiSikora
Copy link
Contributor

MiSikora commented May 8, 2024

The badge counter does not respond to font size. Is this intended?

Screenshot_20240508-152438

@mebarbosa mebarbosa self-requested a review May 8, 2024 13:47
@ashiagr
Copy link
Contributor Author

ashiagr commented May 9, 2024 •
edited

@mebarbosa 👋

I've reduced badge offset in 343cac4

@MiSikora 👋

The badge counter does not respond to font size. Is this intended?

It is intentional as on a large display, the grid items remain the same size but the badge will cover the artwork too much.

@ashiagr ashiagr enabled auto-merge (squash) May 9, 2024 05:30
@ashiagr ashiagr merged commit 0455709 into main May 9, 2024
11 of 13 checks passed
@ashiagr ashiagr deleted the task/2081-podcasts-grid-design branch May 9, 2024 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-gonzalez-a8c david-gonzalez-a8c Awaiting requested review from david-gonzalez-a8c

@mebarbosa mebarbosa Awaiting requested review from mebarbosa

Assignees
No one assigned
Labels
unit-tests-exemption Up Next
Projects
None yet
Milestone
Future
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ashiagr @dangermattic @MiSikora @mebarbosa