Skip to content

Intercept touch event to task list checkboxes to handle it before the cursor is moved - #1096

Merged
khaykov merged 11 commits into
trunkfrom
improve-checkboxes
Jan 28, 2025
Merged

Intercept touch event to task list checkboxes to handle it before the cursor is moved#1096
khaykov merged 11 commits into
trunkfrom
improve-checkboxes

Conversation

@danilo04

@danilo04danilo04 commented Jan 23, 2025

Copy link
Copy Markdown
Contributor

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

@khaykovkhaykov left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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

@khaykovkhaykov left a comment

Copy link
Copy Markdown
Contributor

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 trunkJan 28, 2025
@khaykov
khaykov deleted the improve-checkboxes branch January 28, 2025 15:55
Sign up for freeto 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.

2 participants

@danilo04@khaykov