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

Update Compose BOM to 2024.02.00. Fixes #1218 #1220

Merged
merged 22 commits into from
Feb 26, 2024
Merged

Conversation

dturner
Copy link
Collaborator

@dturner dturner commented Feb 19, 2024

What have I done and why?

  • Updated the Compose BOM from 2023.10.01 to 2024.02.00 (Compose 1.6.2)
  • Updated deprecated/changed Material3 components in line with recommended guidance
  • Fixed a couple of minor build warnings
  • Replaced BoxWithConstraints with Box
  • Changed font heights, alignment and spacing so that the current UI layouts stay the same. In Compose 1.6 (introduced by this BOM) includeFontPadding defaults to false. This causes some unwanted layout changes. To preserve the current UI I had to tweak the typography styles using LineHeightStyle and make some slight layout changes. The new layouts aren't a pixel perfect match (as can be seen in the screenshot diffs) but they are close enough so that no-one should notice.

Notes on screenshot diffing
Updating the layouts to match previous screenshots was a time consuming process. In case anyone else needs to do it, here's what I did:

  • Clone the repo, checkout the commit/branch which contains the "old" screenshots i.e. the ones which are considered to be correct
  • Record new screenshot tests./gradlew recordRoborazziDemoDebug --rerun-tests. Note the --rerun-tests ensures that new screenshots are actually recorded, without it I would sometimes find that they weren't updated despite code changes
  • Copy the screenshot(s) folders to "screenshots-old" in the same parent folder - this is to move them out of source control
  • Checkout your current branch (the one which will contain the new screenshots), make UI changes and record new screenshots
  • Use Beyond Compare 4 to do a folder comparison between the old and new screenshots folders. It has some extremely useful features like being able to offset one image on the other and do a pixel by pixel comparison with surrounding pixels:
image

dturner and others added 9 commits February 19, 2024 16:29
Change-Id: I838b81bf8e7fd7cb135f422653a8d8041829b28b
Change-Id: I8b243ef88c1cc8be83f5fbeaee6a2576c453a062
Change-Id: Iea0401e28ae0e884511800652fde0f1896642aa0
Change-Id: Ida974df8fd4b1aba6f9525ef5335a6cbe02ddb18
Change-Id: I8d19c437fc6e2e2198af5ac871c3f7813d280923
…lt since Compose 1.6.0-alpha01

Change-Id: Ifd291b247f848cea43e084cc0fb99917e47e26eb
@JoseAlcerreca
Copy link
Contributor

Looks like if you re-run a job after the screenshots were generated, it fails when trying to commit the new ones. Can you add another commit or force-push without the 9e64 commit?

@LinX64
Copy link
Contributor

LinX64 commented Feb 19, 2024

Looks like if you re-run a job after the screenshots were generated, it fails when trying to commit the new ones. Can you add another commit or force-push without the 9e64 commit?

As a note, a rebase is required to apply the changes from your previous merged PR about CI to solve the emulator issue, please see here.

@dturner dturner added the version update This is related to a library, API or SDK update which requires some work. Not just a version bump. label Feb 20, 2024
@dturner dturner self-assigned this Feb 20, 2024
dturner and others added 10 commits February 20, 2024 16:31
Change-Id: If6b292af514df435d7bd233bd634657904c67d6b
Change-Id: I4011205f8a07c418653ee463d7355fe1e4ff6f44
Change-Id: I8e298da139b19721b90ae778e7ab184f600d0bd7
Change-Id: I6894836a7cd3a6139511a5d1ac0a96702a265186
Change-Id: I68cb9bd989a809e5db1bca3157c2c978ad8c8906
* main:
  🤖 Updates screenshots
  Make app screenshots use the app theme

Change-Id: I2f8d81b1bad7c04e26db0175fd7562d44de51ff2
Change-Id: I7dec3672b9efc3524371811743b06efc1e113a36
Copy link
Contributor

@alexvanyo alexvanyo left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed description for the text style changes! Resulting screenshot changes look good, either barely noticable or strictly an improvement (like the navigation bar one at large font scales)

Change-Id: Ia404cde7f6b555234bef4b27058fabd4471ea3c6
@dturner dturner merged commit 76d0e43 into main Feb 26, 2024
4 checks passed
@dturner dturner deleted the update-compose-bom branch February 26, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version update This is related to a library, API or SDK update which requires some work. Not just a version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants