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

setTabCloseEventListener fails if tab does not use close button. #62

Closed
rpeaugh opened this issue Jan 11, 2022 · 3 comments
Closed

setTabCloseEventListener fails if tab does not use close button. #62

rpeaugh opened this issue Jan 11, 2022 · 3 comments

Comments

@rpeaugh
Copy link

rpeaugh commented Jan 11, 2022

Our application has one permanent tab, the home tab if you will, and it does not require the close button on it. You can see the error in the console when the page opens. Simply leave out the line in your html that says:

<div class="chrome-tab-close"></div>

I resolved this as follows:

setTabCloseEventListener(tabEl) {
    if (tabEl.querySelector('.chrome-tab-close') != null) {
        tabEl.querySelector('.chrome-tab-close').addEventListener('click', _ => this.removeTab(tabEl))
    }
}
@adamschwartz
Copy link
Owner

adamschwartz commented Jan 29, 2022

Thanks @rpeaugh! This can also be solved without changing the library JS by using some additional CSS:

.chrome-tab-close {
  display: none !important
}

One advantage of this approach is it might allow you to easily toggle the closeability of a tab. This could be useful for #64 for example.

@rpeaugh
Copy link
Author

rpeaugh commented Jan 29, 2022

Hey Adam,

Thanks for the response. The CSS could be useful as you described.

I was thinking about the pinned tabs issue when I made that simple code change. With the code change, one can leave off the draggable and close options and the tab is "essentially" pinned. The one remaining issue is that, although it cannot be dragged, a draggable tab can be dragged over it, forcing it to move. This is a minor issue, but if draggabilly were updated to not be draggable over icons that are not draggable, then the first tab would remain pinned in that location. I would think that most developers, such as myself, are wanting to pin a Home/Welcome tab as the first tab. And as long as all pinned tabs were adjacent, and starting from the first tab, then they would all remain in those positions. On a side note, the code change eliminates the error that occurs when you leave off the chrome-tab-close class. Which, in my opinion, makes the code more friendly.

Now, if one were trying to create some tabs such as:
[pinned] [draggable] [draggable] [pinned] [draggable] [draggable]
That would definitely be an issue. Keeping that fourth tab in the fourth position, I cannot fathom a reason someone would want to do this. Though I suppose you would have to make the draggable tabs only draggable between the pinned tabs. Otherwise, the pinned tab would shift when a draggable tab was moved across it and it would no longer be in the fourth position.

@adamschwartz
Copy link
Owner

adamschwartz commented Jan 31, 2022

Thanks @rpeaugh for this thoughtful context.

I could imagine an app that doesn’t need the concept of pinning tabs, but still wants to hide the close button on all or some tabs. Someone might simply want to hide the close buttons for aesthetic reasons while still supporting tab closing via a keyboard shortcut, for example.

So we’ll want to support the Google Chrome notion of pinned tabs (#64) while also keeping the library flexible to support the arbitrary hiding of a close button as needed by an app developer. This shouldn’t be too complicated to achieve but it may require a destructive version bump and/or motivate the need for more substantial documentation. All of which is possible, but just work I can’t commit to at the moment. Always open to a PR ;)

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

No branches or pull requests

2 participants