Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Improved dnd for bookmarks manager (dnd refactor) #7129

Closed
wants to merge 1 commit into from

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Feb 8, 2017

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Resolves #6246

Part of this PR is also a refactoring, so that we can use dnd in other components.

@bsclifton Can you please check out my first commit Removed dnd in url bar. In 5837fc5 you added dnd for URLIcon. I don't see why this should be here (couldn't find anything about it in parent issue). Is it ok to remove it? If you dnd search icon in the current release you get error like this:
image

@bbondy
Copy link
Member

bbondy commented Feb 9, 2017

all browsers allow you to drag that to bookmarks bar to create a bookmark, I don't think removing it is something we should do.

@NejcZdovc
Copy link
Contributor Author

@bbondy I am talking about URL bar, not bookmarks bar. This code:

onDragStart={this.onDragStart}
draggable
.

Try dnd search icon and you will see an error.

image

@bbondy
Copy link
Member

bbondy commented Feb 9, 2017

Yes I'm talking about the same, but you can take the space to the left of the URL and drag to the bookmarks bar to create a new bookmark from the site you're on.

@bsclifton
Copy link
Member

bsclifton commented Feb 9, 2017

like @bbondy said, it's expected to be able to drag that are (which works great for me in master and our latest release too). That is actually how I store all of my bookmarks 😄

@NejcZdovc the error (as shown in your screenshot) appears to a regular mutable JavaScript object when this control is expecting an Immutable.js object

@NejcZdovc
Copy link
Contributor Author

@bbondy @bsclifton If I am honest I was not aware this was possible. I am always creating bookmarks from tabs. I tried it and it's working, so yes we need to leave it as it is, only fix this error.

@bbondy
Copy link
Member

bbondy commented Feb 9, 2017

ya I always create my bookmarks using it too, previously browsers used to put a site's favicon in the urlbar, but that led to sites using lock icons as their favicon which is a security problem, so browsers stopped displaying the lock there, but still allow dragging from that spot.

@bbondy
Copy link
Member

bbondy commented Feb 11, 2017

just a heads up that DND is going through some changes due to tab tear off work.

@NejcZdovc
Copy link
Contributor Author

Yeah I saw your PR and that's why I put a breaks on this PR and split sites PR.

@bsclifton
Copy link
Member

Closing this PR for the moment, since it's blocked. Please do reopen when you have time to revisit 😄 Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants