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

feat: Visual checkbox toggle #131

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kaymmm
Copy link
Collaborator

@kaymmm kaymmm commented Oct 8, 2022

this addresses #89

Calling :'<,'>ToggleCheckboxVisual with a visual selection toggles all the checkboxes in the selection.

Adds the mapping vnoremap <leader>x to call the new function.

It also tweaks the toggle checkbox function to accept a line number rather than have it operate on the current line.

@dkarter
Copy link
Member

dkarter commented Oct 8, 2022

This looks good to me!

Not sure why the tests are not passing though - worth investigating. I've fixed the timeout issues in CI so this is likely a real failure.

Locally I was getting better results when using V (for LINE Visual mode as opposed to v)

    vim.normal 'j'
    vim.normal 'V'
    vim.normal '2j'
    vim.command 'ToggleCheckboxVisual'

But it still didn't select the 3rd checkbox - not sure if that's a bug.

Side note:
I was thinking of changing the default toggle checkbox mapping though to <leader>xb - the reason is that the current mapping has a delay if the user already has <leader>x? where ? stands for any other character.

Vim will wait in this case for the next character for a bit which can be annoying. Setting it as <leader>xb solves the issue.

@kaymmm
Copy link
Collaborator Author

kaymmm commented Oct 8, 2022

The test is failing correctly. I just realized that the plugin is working "correctly" in that it toggles the parent (on) first, which toggles the child (on), but then the child gets toggled (off), which triggers the parent to toggle (off). It's unintuitive and the wrong behavior for what one would expect. Will need to rethink the logic here before merging.

@kaymmm
Copy link
Collaborator Author

kaymmm commented Oct 9, 2022

I can't figure out why the test I put together for this is failing, but the function seems to be working correctly now. I can't tell if the input I'm sending to the test is bad or if there's some reason why the function is not working as intended.

@harshad1
Copy link
Collaborator

harshad1 commented Mar 8, 2024

Could you resolve conflicts and check if the tests still fail? If they do, could you post the failures here?

I can help debug and get this in

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.

3 participants