Skip to content

Conversation

@planarvoid
Copy link
Contributor

@planarvoid planarvoid commented Aug 2, 2022

Fix

This PR fixes an issue when the placeholder size was different (by a little bit) than what was defined in the adapter. This was mostly visible on smaller placeholders. I think only the last placeholder of the document was affected.

In addition it fixes the width so that the cursor doesn't overlap the placeholder

Test (it's better to test this with the PR where it's used)

  1. Setup placeholders
  2. Add 2 placeholders to the EXAMPLE
  3. Add logging of actual height to the code
  4. Check the height is fixed

Review

@khaykov

Make sure strings will be translated:

  • If there are new strings that have to be translated, I have added them to the client's strings.xml as a part of the integration PR.

@planarvoid planarvoid added the bug label Aug 2, 2022
@planarvoid planarvoid requested a review from khaykov August 2, 2022 09:43
@planarvoid planarvoid self-assigned this Aug 2, 2022
Copy link

@Teelair Teelair left a comment

Choose a reason for hiding this comment

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

I tested this using Day One Android on the fix/too-big-audio-player branch.

To test, I made four entries each containing photos, audio files, videos, and a PDF. I took screenshots of the entries looked in both read and edit mode before applying this PR's changes, and then after.

The most noticeable issue where audio placeholders were visibly larger than others has been fixed, but also other placeholders have had their size slightly altered. It is not by much so I feel like this is not a big deal within the context of Day One, but because I am not sure of what all is using Aztec, I'll leave an approval and leave it up to you to merge this if that sounds okay to you @planarvoid!

:shipit:

Copy link
Contributor

@khaykov khaykov left a comment

Choose a reason for hiding this comment

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

Approving since @Teelair gave a 🟢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants