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

Added multiselect to ListView #40

Merged
merged 6 commits into from
Aug 30, 2022

Conversation

ajmeese7
Copy link
Member

@ajmeese7 ajmeese7 commented Aug 9, 2022

Adds some additional functionality to the existing #33, which is meant to close out #32.

This implementation has more feature-parity with typical desktop file manager applications in that the ctrl and shift keys now have separate useful functionality. The ctrl key allows for the selection of multiple individual files, whereas the shift key allows for a selection of a range of files.

src/components/ListView.js Outdated Show resolved Hide resolved
src/components/ListView.js Outdated Show resolved Hide resolved
@ajmeese7
Copy link
Member Author

Do you want changes to be made to try to satisfy the CodeClimate CI?

@andersevenrud
Copy link
Member

Do you want changes to be made to try to satisfy the CodeClimate CI?

Don't really have to worry about those unless you're met with a bunch of 'em. But you can look at it if you want to.

@ajmeese7
Copy link
Member Author

In this case I don't believe the changes are necessary, the "complexity" really isn't all that complex in my opinion. Is there anything else you want to see out of this PR?

Copy link
Member

@andersevenrud andersevenrud left a comment

Choose a reason for hiding this comment

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

I haven't found the time to test this myself, but from a code standpoint, this looks OK to me.

I could publish this with a @next tag on npm if you want. I wanna test this myself, but don't think I really have enough time to thoroughly do it until Thu/Fri.

@andersevenrud andersevenrud merged commit 92fb5bc into os-js:master Aug 30, 2022
@andersevenrud
Copy link
Member

I could publish this with a @next tag on npm if you want

I went ahead and did that. So 4.2.0-rc1 is tagged with next so that it's not picked up by npm updates.

@ajmeese7 ajmeese7 deleted the feature/listview-multiselect branch August 30, 2022 21:50
@ajmeese7
Copy link
Member Author

Sounds good, when you manually test please let me know if you encounter any issues and I would be happy to open another PR to fix them.

Additional note, this should close out #32 and #33 now that it is merged.

@andersevenrud
Copy link
Member

I'd hold up closing related issued until it's published into current.

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.

2 participants