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

Tabs with elements in the header #32

Closed
mikeburgh opened this issue Nov 2, 2022 · 12 comments
Closed

Tabs with elements in the header #32

mikeburgh opened this issue Nov 2, 2022 · 12 comments

Comments

@mikeburgh
Copy link

Firstly, great components!!

I found a small issue with the tab's.

If you have other elements inside the header, eg:

...
then clicking in the tab does not change the selected tab.

I think the reason is due to the target element not having the dataset.index value (as it's the div not header element) as per the code here: https://github.com/bendera/vscode-webview-elements/blob/master/src/vscode-tabs.ts#L57

@bendera
Copy link
Member

bendera commented Nov 2, 2022

Hi,

I'm glad you like it!

It seems this hard-coded "header" selector causes this bug:
https://github.com/bendera/vscode-webview-elements/blob/master/src/vscode-tabs.ts#L101

I'm going to make this selector more flexible. Anyway, could I ask you, why you use a div element instead of a header here? I would like to see your code if it's public.

@mikeburgh
Copy link
Author

Code is not public yet. The div was just an example that demonstrated it in the most extreme case (header element completely filled). I actually found it by adding a badge to the tab after some text, and noticed clicking the badge prevented the switch.

The other idea I had for a fix was to change the code to search the path for the header element and then read the dataset:
event.composedPath().find(this)!.dataset.index

bendera added a commit that referenced this issue Nov 3, 2022
Select the connected tab panel when a slotted element is clicked in the tab header (#32)
@bendera
Copy link
Member

bendera commented Nov 3, 2022

Fixed in 0.7.1

@bendera bendera closed this as completed Nov 3, 2022
@mikeburgh
Copy link
Author

Works great!!

@bendera
Copy link
Member

bendera commented Nov 7, 2022

Hi,

I published a pre-released version (0.8.0-pre.0), it's worth a try. I've rewritten the Tabs component, it's fully WAI-ARIA compatible. If you would like to see the new examples, check out the rel-0.8.0 branch, then run npm ci and npm run docs:start

@mikeburgh
Copy link
Author

NICE!

Also noticed you added badge, I was going to tidy my hacked version up and send you a PR for it.. yours is nicer.. textfield looks very handy as well!

I tried out the pre release, got an error bundling cause of the capital I in the from of the import : https://github.com/bendera/vscode-webview-elements/blob/rel-0.8.0/src/vscode-tabs.ts#L8

@mikeburgh
Copy link
Author

One thing I noticed on the new tabs.. you have text-overflow: ellipsis; set, but it does not seem to work and the tab just gets wider with long text.

I added it by placing the text inside the header in it's own div, and setting text-overflow and max-width on it:

<div>${title}</div>
<vscode-badge variant="counter">${this.badge}</vscode-badge>
div {
  display: inline-block;
  max-width: 200px;
  white-space: nowrap;
  overflow: hidden;
  text-overflow: ellipsis;
}

but that breaks up the alignment with the badge a little (which I can fix)
Screenshot 2022-11-07 at 7 23 14 PM

@mikeburgh
Copy link
Author

Sorry, also just found.. it looks like you are missing the call to this._setActiveTab(); inside _onHeaderSlotChange.. so when you set the selected index outside, it does not get picked up when items are added as it did before.

@bendera
Copy link
Member

bendera commented Nov 8, 2022

Thanks for the feedback!

I've fixed the text-overflow issue. Just set the max-width on the tab-header.

when you set the selected index outside, it does not get picked up when items are added as it did before.

I could reproduce it. This happens when the [slot=header] attribute is missing. Since it is easy to fix and the public API would be cleaner, I changed the code to handle this case.

0.8.0-pre.1 has been bumped.

@mikeburgh
Copy link
Author

The import of uniqueId still has the wrong case, but other changes are working great!

I think I will still need to do my own text trimming in a sub node in the tab for just the text, otherwise while the current way trims it, it also hides the badge/close icons. All good I can sort that.

Appreciate the updates.

@bendera
Copy link
Member

bendera commented Nov 8, 2022

The import path is fixed, a new release has been published.

@mikeburgh
Copy link
Author

Working great now!

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