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

Feature: more drag-able elements #66

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

clayboone
Copy link

Grab-and-Drag (pre Quantum, I'm not sure about now) was able to drag on links, images, etc. and I like that ability. So I've added it to SA.

Let me know what you think, and what the defaults should be.

options page seemed kind of lop-sided with this element
add margin-left to labels if they need more spacing
Changed "show debug noise", it's now a first-class citizen with a capitol 'S' just like the other checkboxes.

Added "Tip:" to cursor changer option to match the other class="tip" div.
i may take these out of the array later
this can be done better
instead of just targets with button roles.
@davidparsson
Copy link
Owner

Thanks for your pull requests! I'll be busy for a few days but I'll try to find the time to take a look as soon as possible.

@clayboone
Copy link
Author

clayboone commented Jan 19, 2018

Thanks! No rush, just trying to learn a bit of JS and I like this extension.

edit: This particular PR is kind of a mess. Sorry.

@davidparsson
Copy link
Owner

There's a merge conflict that needs to be resolved. Would you mind doing that?

And if you're up it, I'd appreciate if you could squash a few of the commits to make the history a bit cleaner. If not, let me know and I'll review the code as is.

@davidparsson
Copy link
Owner

Never mind the conflict, I could fix it myself.

@clayboone
Copy link
Author

I wasn't sure you would want to merge this one as-is because of some of the changes. Namely, all buttons acting the same now instead of having different elements behave differently. After I packed/installed this locally, the bug of not being able to actually click links when using left mouse went away.

I'm pretty sure the conflict here was needing to move down some code in background.js due to me adding another local_storage option to the top of the file. Glad you got it figured out.

@davidparsson
Copy link
Owner

All right. I'm still not entirely sure, but I found that out after commenting. The thing that makes me hesitate is that the configuration of the elements might introduce more complexity than it adds value. What are your thoughts about that? I'm sure we can work something out.

@clayboone
Copy link
Author

The options could definitely be simplified...

We could reduce it to just two extra options under the current "Don't drag when clicking on text" checkbox. That would be much simpler, until you set some weird configuration, like only being able to drag on links that aren't images and not regular text. That's why the options currently work the way they do, even if ugly.

The whole mess could be a drop down or radio-box choice with varying amounts of draggable elements like "Only whitespace; Whitespace and text; Everything"

Or packing all of it into a single extra "Don't drag on links or images" checkbox option. That option would need to be on by default though to maintain current behavior. It also might not make some sense if dragging on links is enabled, but dragging on text is not.

How about changing the idea of separation from text and links, to text and images. "Don't drag...on text" would include text links, and "Don't drag...on images" would include image links.

I think I like that last one as it only adds one option and neither one needs to implement graying the other out, though it may require adding either another Tip or some title text explaining that "text includes text links."

Does any of this sound good?

@davidparsson
Copy link
Owner

That might work, but I'll have to think a bit more (when I've left work). 🙂

@davidparsson davidparsson mentioned this pull request Apr 9, 2018
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