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: Toggle range of checkboxes while pressing shift key #8

Merged
merged 6 commits into from
Feb 18, 2020
Merged

Feat: Toggle range of checkboxes while pressing shift key #8

merged 6 commits into from
Feb 18, 2020

Conversation

plibither8
Copy link

Hi! Great extension :)

This is a feature that I really thought would be useful, I required it myself. I have come up with an implementation in this PR, the code might be very rough - do tell me where to change it.

Thanks!

@guytepper
Copy link
Owner

Woah, thanks a lot! Great work 🙂 That'd be a super useful feature.

I've made some small code styling changes.
Can you please bump the version in the manifest.json file to 1.4.0 so I can merge the PR and update the extension?

Some improvements can be made to the range selection mechanism later on - I'll open an issue for that. If you'd like to take responsibility for it, let me know :)

@plibither8
Copy link
Author

Hi! I took a look at the desired selection mechanism detailed in #9. This looks pretty simple to implement, I'd be happy to take it up.

@plibither8
Copy link
Author

@guytepper I think I have made it consistent with the mechanism as shown done by 'checkboxes.js'. Please take a look. Thanks :)

@guytepper
Copy link
Owner

Thanks! It almost works perfectly - is it possible to make it uncheck a range of checkboxes, from the last one selected? (same as the behavior in checkboxes.js)

2019-09-28 15 06 28

@plibither8
Copy link
Author

Hi, apologies for the delay! I took a look at the code checkboxes.js was using and implement (hopefully) the same mechanism. It'd be great if you could check it out. Thanks!

Copy link
Owner

@guytepper guytepper left a comment

Choose a reason for hiding this comment

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

Thanks! It works perfectly.
Sorry it took me so much time to review. Just added some little notes.

let startIndex;
// Where the range selection should end
let endIndex;
// Value that firstIndex initially had
Copy link
Owner

Choose a reason for hiding this comment

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

Did you mean startIndex? Also, maybe add an explanation for the purpose of the variable :)

}

const newRange = checkboxes.slice(startIndex, endIndex + 1);
previousRange.map(cb => (cb.checked = resetValue));
Copy link
Owner

Choose a reason for hiding this comment

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

Seems like previousRange is being declared globally on line 65.
Can you add a local declaration near the other variable declarations?

}

const newRange = checkboxes.slice(startIndex, endIndex + 1);
previousRange.map(cb => (cb.checked = resetValue));
Copy link
Owner

Choose a reason for hiding this comment

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

Also, can you please add a comment explaining what's being done in lines 56-59?
I think a comment will make it easier to grasp :)

@guytepper guytepper changed the base branch from master to checkboxes-range February 18, 2020 19:49
@guytepper guytepper merged commit de77268 into guytepper:checkboxes-range Feb 18, 2020
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