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(browse): fix regression showing borders #17761

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

david-allison
Copy link
Member

@david-allison david-allison commented Jan 7, 2025

Fixes

Approach

Use DividerItemDecoration

How Has This Been Tested?

API 34 emulator

Screenshot 2025-01-07 at 22 58 48 Screenshot 2025-01-07 at 22 59 15

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

let there be dividers

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Actually - wait - looked at it locally

in light theme, pretty light, could be darker
in plain theme, mmm they look pretty light to me
in dark theme, they are not quite dark enough IMHO
in black theme they are basically invisible

Is there a way to have them be color-of-font but half-saturation or similar? Or maybe just whatever they were before so there's no bikeshedding 😆

Right now they exist but aren't strong enough in most themes IMHO

image

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Jan 7, 2025
`cardBrowserDivider` was defined as a color
but we sometimes provided a reference

Fixes 17751
@david-allison david-allison removed the Needs Author Reply Waiting for a reply from the original author label Jan 7, 2025
@david-allison
Copy link
Member Author

david-allison commented Jan 7, 2025

Fixed. Great catch

PS: That change was ridiculously complex :/. There were issues as cardBrowserDivider was defined as a color, but provided with a reference

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I still had an old parallel build on my mobile so I was able to compare it vs this running on an emulator. Looks exactly the same to me now in all 4 themes

@mikehardy mikehardy added this pull request to the merge queue Jan 8, 2025
@mikehardy
Copy link
Member

I'm merging straightaway because fixing a regression with a clean fix that achieves parity doesn't really leave much for anyone else to do, time best spent elsewhere IMHO :)

Merged via the queue into ankidroid:main with commit 17da2ec Jan 8, 2025
9 checks passed
@github-actions github-actions bot added this to the 2.21 release milestone Jan 8, 2025
@david-allison david-allison deleted the 17751 branch January 8, 2025 03:45
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.

Browser: Show borders between adjacent cards
2 participants