Skip to content

Conversation

@danilo04
Copy link
Contributor

@danilo04 danilo04 commented Jan 23, 2025

Fix

This PR improves the UX of task lists checkboxes touch events. The changes are a little hacky but that is how the current solution to work with checkboxes work currently.

Test

  1. Check that touch events work for all kind of formats
  2. Tap on the ordered lists numbers and see that the focus and cursor are moved to that item
  3. Tap on the checkboxes for task lists and check that they don't move the cursor but the action of check/unchecked is performed.

Review

@planarvoid

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.

@danilo04 danilo04 requested a review from planarvoid January 23, 2025 16:02
@danilo04 danilo04 marked this pull request as ready for review January 23, 2025 16:02
@danilo04 danilo04 changed the title Intercept touch event to handle it before the cursor is moved Intercept touch event to task list checkboxes to handle it before the cursor is moved Jan 23, 2025
@danilo04 danilo04 requested a review from khaykov January 24, 2025 22:35
@khaykov khaykov self-assigned this Jan 27, 2025
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.

Thanks, @danilo04 ! It works well. Maybe we can reduce the padding to 15, what do you think?

I also have a suggestion for preserving a cursor. Let me know what do you think :)

x,
totalPaddingStart
) == true) {
refreshText()
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a whole bunch of code here that was supposed to preserve a cursor, but the only thing that was actually doing was just passing false into refreshText.

Suggested change
refreshText()
refreshText(false)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Man, that is what happens when you don't read the method's signature. Thank you for the pointer.

@danilo04
Copy link
Contributor Author

Thank you for the recommendation @khaykov, I implemented both changes.

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.

Looks good, thank you!

@khaykov khaykov merged commit 8ccb5d3 into trunk Jan 28, 2025
14 checks passed
@khaykov khaykov deleted the improve-checkboxes branch January 28, 2025 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants