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

Fix #4684: Revert (most of) #4587 #4686

Merged
merged 4 commits into from
Nov 1, 2022
Merged

Fix #4684: Revert (most of) #4587 #4686

merged 4 commits into from
Nov 1, 2022

Conversation

BenHenning
Copy link
Sponsor Member

@BenHenning BenHenning commented Oct 31, 2022

Explanation

Fixes #4684

This PR reverts #4587. I think the change of match_parent to wraps_content caused the promoted story container to grow for larger images, and this wasn't caught until we tried the changes with production assets. I noticed other places in the PR that might have the same issue, so rather than trying to make sense of everything in order to properly fix it & add tests, I opted to revert the change since the regression is beta-blocking (even though it wasn't a clean reversion and required manual changes).

Note that the reversion includes keeping the changes to recently_played_fragment since otherwise changes in RecentlyPlayedFragmentTest fail, and the change seems unlikely to cause any problems with respect to potential thumbnail sizing issues.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

There's not much to show here--see the issue for context on the problem. The post-reversion just reverts things back to their previous state (i.e. no regression).

@BenHenning
Copy link
Sponsor Member Author

/cc @vrajdesai78

@BenHenning BenHenning changed the title Fix #4684: Revert #4586 Fix #4684: Revert #4587 Oct 31, 2022
One was an incorrect manual fix during reversion, and the other is
keeping a fix from the original PR that would otherwise cuase an
existing test suite to fail.
@BenHenning BenHenning changed the title Fix #4684: Revert #4587 Fix #4684: Revert (most of) #4587 Nov 1, 2022
@BenHenning BenHenning marked this pull request as ready for review November 1, 2022 05:59
@BenHenning
Copy link
Sponsor Member Author

@seanlip PTAL. Since this is mostly a clean reversion, I think it's fine to submit without @rt4914's review (plus I suspect he'll look at it with Vraj given the base PR). Note that CI passed before syncing to latest develop, so probably don't need to block on that for approval.

Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

Just one note? Otherwise LGTM

app/src/main/res/layout/app_language_activity.xml Outdated Show resolved Hide resolved
@seanlip seanlip assigned BenHenning and unassigned seanlip Nov 1, 2022
@vrajdesai78
Copy link
Contributor

vrajdesai78 commented Nov 1, 2022

@BenHenning I am good with the revert of changes related to thumbnail image. I have made that change previously because accessibility scanner is giving a suggestion to improve scaling with "Fixed witdh"

image

@BenHenning
Copy link
Sponsor Member Author

Thanks @seanlip! Enabling auto-merge for once CI finishes.

@vrajdesai78 we'll need to find a way to satisfy the constraints such that it doesn't break the app (wrap_content isn't the correct thing to do here).

@BenHenning BenHenning enabled auto-merge (squash) November 1, 2022 07:07
@BenHenning
Copy link
Sponsor Member Author

Ah forgot that I need to force-merge since technically this requires @rt4914's codeowners approval.

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.

Promoted story cards fit images rather than constraining them
4 participants