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

Update Drag&Drop code #20

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@nmorey
Copy link

commented Apr 12, 2019

  • Allow moving containers (Issue #7)
  • Allow moving tab in another container in a given position

Signed-off-by: Nicolas Morey-Chaisemartin nmoreychaisemartin@suse.com

@nmorey nmorey force-pushed the nmorey:dev/master/move-containers branch from e86ed80 to 664d984 Apr 12, 2019

Update Drag&Drop code
- Allow moving containers (Issue #7)
- Allow moving tab in another container in a given position

Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>

@nmorey nmorey force-pushed the nmorey:dev/master/move-containers branch from 664d984 to 9fa79c1 Apr 12, 2019

@maciekmm
Copy link
Owner

left a comment

Awesome work, thanks for your contribution.
I have added some comments.

We might think about saving the container ordering in some sort of storage.

border-bottom: 1px solid #eee !important;
}

#pinned-tabs .container-tab-dragged-over {
border-right: 1px solid #eee;

This comment has been minimized.

Copy link
@maciekmm

maciekmm Apr 21, 2019

Owner

This border-right was used when moving tab from unpinned to a pinned state. Now there's no indicator where the tab will land.

This comment has been minimized.

Copy link
@nmorey
@@ -48,12 +48,20 @@ class AbstractTabContainer {
this.element.addEventListener('dragleave', (e) => {
if (!e.currentTarget || !e.currentTarget.classList) return
e.currentTarget.classList.remove('container-dragged-over')
})
e.currentTarget.classList.remove('tab-dragged-over')

This comment has been minimized.

Copy link
@maciekmm

maciekmm Apr 21, 2019

Owner

Could you change the indention to spaces? The codebase uses space indention, and while I prefer tabs I would rather not mix the two.

This comment has been minimized.

Copy link
@nmorey

nmorey Apr 22, 2019

Author

Sure.

@nmorey

This comment has been minimized.

Copy link
Author

commented Apr 22, 2019

We might think about saving the container ordering in some sort of storage.

It would make a lot of sense. Just switching to bookmarks in the sidebar and going back to the tabs resets the order.
But I'm bery new to JS development so I'm spending way too much time trying to figure out what API to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.