Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve merging of tags by simple drag and drop #144 #154
Improve merging of tags by simple drag and drop #144 #154
Changes from 1 commit
1485e99
2068f71
c40585b
d421f93
4f08146
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a react expert but I think global mutable state are frowned upon? Is it not possible to do this with a
React.useState
instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not an expert either^^
I have however extracted it out now, since that functionality is pretty much the same for the drag&drop functionality for assigning bookmarks to lists (next PR ;-) ), so it makes sense to reuse it.
It is now more encapsulated, so I guess that concern should be gone, but let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, after learning a lot, i found this:
useState needs to be used for the change detection to work, but the drag&drop handlers are quite slow to execute and react-draggable is not implemented that well. Due to the processing overhead, the
My followup PR for also implementing drag&drop for assigning bookmarks to lists has therefore some changes:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we pass
onDragOver
toTagPill
and remove that div?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
turns out the onDragOver is not actually needed, so I removed it.
If I remove the div though, it is suddenly not possible to drag it anymore. I am not sure why though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those can probably be
shadcn
toggles (https://ui.shadcn.com/docs/components/toggle) and can probably be aligned right in the page.If UI design is not your expertise, I'm happy to implement it myself after merging your PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this now
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.