Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

[FIX]: Image occlusion cards created on desktop app now fit to width

Open RedLexa opened this issue 1 year ago • 12 comments

Purpose / Description

This commit applies the suggested solution in #15235, ensuring Image Occlusion cards created in the desktop app fit to width.

Fixes

  • Fixes #15235

Approach

Applying the correct styling with !important to make sure it overwrites whatver came over from the Desktop app that was causing issues.

How Has This Been Tested?

I ran this with multiple image sizes, creating them in the desktop app and then opening then in AnkiDroid while in landscape mode.

Checklist

  • [X] You have a descriptive commit message with a short title (first line, max 50 chars).
  • [X] You have commented your code, particularly in hard-to-understand areas
  • [X] You have performed a self-review of your own code

RedLexa avatar Apr 05 '24 19:04 RedLexa

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

welcome[bot] avatar Apr 05 '24 19:04 welcome[bot]

Thanks! Could you provide before/after screenshots in portrait and landscape?

Did you also resolve: https://github.com/ankidroid/Anki-Android/issues/15235#issuecomment-2004609689

david-allison avatar Apr 05 '24 19:04 david-allison

Thanks! Could you provide before/after screenshots in portrait and landscape?

Did you also resolve: #15235 (comment)

I did resolve https://github.com/ankidroid/Anki-Android/issues/15235#issuecomment-2004609689 ! I wasn't putting !important on the right properties, that fixed it. As for the screenshots, here they are: BEFORE Hacksaw RIdge normal Hacksaw RIdge em Landscape AFTER Hacksaw final  Landscape Hacksaw final normal

RedLexa avatar Apr 05 '24 20:04 RedLexa

And to confirm, that's the intended outcome (as all the text isn't occluded)

david-allison avatar Apr 05 '24 20:04 david-allison

And to confirm, that's the intended outcome (as all the text isn't occluded)

Yes! That was just a placeholder occlusion I placed to test if it was working. (You aren't to first to point out how slightly annoying it is that the occlusion just barely covers all the text, I'll try to do better next time :) )

RedLexa avatar Apr 05 '24 20:04 RedLexa

The backside of card hides toggle button,

krmanik avatar Apr 06 '24 05:04 krmanik

Hey everyone. Sorry for the late reply, school got in the way. I'm afraid I have some bad news: the new UI for the card reviewing seems to have messed with something that was making the solution work beforehand. ( https://github.com/ankidroid/Anki-Android/pull/16109#issuecomment-2040596362) Now, the issue seems to be back:

image

However, through my testing, even cards Image Occlusion cards seem created directly on AnkiDroid seem to be having this issue. I'm stumped on what to do here. What changes that were implemented in the last 3 weeks that might mess with the card css? Thank you all again for your patience and sorry for the delay.

RedLexa avatar Apr 28 '24 16:04 RedLexa

@RedLexa are you testing on main? We recently applied a fix for some JS errors in the Reviewer

david-allison avatar Apr 28 '24 16:04 david-allison

@david-allison I'm testing on the branch image-occlusion-fix from my fork. I'm not sure why it's even updated at this point? Must have been one of the accidental fork syncs I did.

RedLexa avatar Apr 28 '24 17:04 RedLexa

You need to manually rebase it onto the current main

david-allison avatar Apr 28 '24 17:04 david-allison

FWIW, this is the CSS that I ~~stole~~ retrieved from inspecting AnkiMobile:

#image-occlusion-container {
    position: relative;
    margin: 0 auto;
    max-height: calc(100vw - 24px) // this was `100vw - var(--io-header)`, which defaults to 24px but can change I think. I don't think that we want to implement something similar
}

#image-occlusion-container img {
    position: absolute;
    top: 0;
    left: 0;
    width: 100%;
    height: 100%;
    max-width: unset;
    max-height: unset
}

BrayanDSO avatar Apr 28 '24 17:04 BrayanDSO

Testing after having rebased had the same result, unfortunately.

@BrayanDSO Where did you get this styling? Was it from inspecting the image occlusion card's styling? The styling I'm applying should overwrite this and fix it, for some reason it isn't.

RedLexa avatar Apr 28 '24 18:04 RedLexa

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

github-actions[bot] avatar May 12 '24 18:05 github-actions[bot]