feat(tabs): added close button and add new tab

[mcoker]

fixes #4757

There is currently no styling or anything associated with the "add new tab" tab - it's just a regular tab with an icon. @mcarrano @mmenestr wdyt?

[patternfly-build]

Preview: https://patternfly-pr-4787.surge.sh

A11y report: https://patternfly-pr-4787-a11y.surge.sh

[mcarrano]

@mcoker thanks for sharing this. I feel like the Close button is a little heavy visually. I know that's our standard icon button styling, but maybe we need something different here? I have a similar feeling about the Add (+) button. @mceledonia can you take a look at this?

[mceledonia]

@mcarrano @mcoker Yeah, I think we can drop down the icon size to small for both close and add, then keep the same padding shown above. Here's a quick mockup of how that looks:

[christianvogt]

IMO the padding to the left and right of the x button seems off as the gap on the right is smaller. But maybe that's ok?

I was testing the addition of the close button to several other variants and there seems to be issues.

[mceledonia]

I'm ok with them being different (left vs right padding) but the padding on the right does feel rather tight on the box variation due to the border being more visible. Any reason not to have separate right-hand padding between box and filled variations? On box I would propose bumping it up a spacer to better balance against the more visible shape of the tab.

[mcoker]

@mceledonia here is the change with extra padding on the right in the box variation

FWIW, here's that change but applying it to the close button globally. I think this looks OK too.

And this is the same change, but using equal left/right padding on the close button, which separates the text and close button a bit.

wdyt?

[mceledonia]

@mcoker I actually think it looks fine on both, thanks for trying it out. These look great to me!

[mmenestr]

The only comment I have (and maybe it's just me) but visually, the text and x icon don't look like they're aligned - ie: the x icon sits a little higher than the text and it feels off to me? Idk if that's something that can be changed though. Looks like they are vertically center aligned when I use inspect which in theory is correct, but wonder what it would look like if the X was a few pixels lower, to bottom align with the text?

[mcarrano]

@mcoker I like this much better now. I think the question that @mmenestr raised is an interesting one. I hadn't noticed it until I read her comment. Is that a function of the X icon being enclosed in a button?

Also, I wondered if we could change the last example to include both the Close button and the Add button together to see what this will really look like. I can't see a case where we would support adding new tabs without the ability to remove them.

[mcoker]

@mcarrano @christianvogt added examples of different variations including secondary.

@mmenestr pushed the close icon down ~2px.

Is that a function of the X icon being enclosed in a button?

@mcarrano yeah, we will need to manually align the close icon to the text since they're in different buttons. Center alignment works fine though since we can center the content in both buttons.

[mcarrano]

@mcoker thanks for adding the additional examples. It does highlight one problem with how this is implemented, however. Looks like the Add button is just another tab that means it gets hidden when the tabs overflow and horizontal scrolling is exposed. I'd expect the Add button to always be visible. The way Firefox handles this is a good model I think.

Note that the Add button exists outside of the tab group following the scroll button so it is always available. Can we do something like that?

[mcoker]

@mcarrano yep we can, though I'll note that I asked about that specifically in one of the planning meetings and we decided it should just be another tab at the end of the list, contained within the scroll buttons - you would need to scroll to the end of the list to see the add tab button. Just want to mention that in case there was a specific reason for that - or maybe I miscommunicated/misunderstood our previous convo 😁

Also assuming we move it out of the list and scroll buttons (when present), should it still look like a regular tab (including the basic tab hover/focus styling) or should it have a more generic style?

[mmenestr]

That looks better @mcoker thank you!
And I kind of agree with Matt that it would maybe be better to move it outside - I don't remember the tab conversation so I can't say why we didn't say that before but, one thing I like about moving it outside, apart from it not getting lost, is that there's also a clearer visual distinction between the end of a tab and the start of the + tab. Right now there's no line or anything between them which feels weird to me.

[christianvogt]

I highlighted the use of the tab as the add button in the original defect #4757

If we can solve the issue of it being hidden while still being present adjacent to the last opened tab (instead of right justified), that would be my preference.

[mcarrano]

@mcarrano yep we can, though I'll note that I asked about that specifically in one of the planning meetings and we decided it should just be another tab at the end of the list, contained within the scroll buttons

Yeah, sorry about that @mcoker . I may have said that without realizing what the implications of that would be.

Also assuming we move it out of the list and scroll buttons (when present), should it still look like a regular tab (including the basic tab hover/focus styling) or should it have a more generic style?

I think the later, because it would be wrong, IMO, for this to be styled as a tab, but not behave like the other tabs. Do you agree @mceledonia @mmenestr ?

If we can solve the issue of it being hidden while still being present adjacent to the last opened tab (instead of right justified), that would be my preference.

I agree

[mcoker]

@mceledonia @mcarrano @christianvogt moved the add button outside of the tabs list (and scroll buttons when present) and added a left border per discussion with design.

with scroll buttons

without scroll buttons

[mceledonia]

Latest looks good!

[mcarrano]

@mcoker the screen grab above looks great. For some reason, the preview still feels like it has more padding for the scroll button, or is that my imagination?

[mcarrano]

Everything looks good. Thanks @mcoker !

[mattnolting]

Nice work! LPTM

[patternfly-build]

🎉 This PR is included in version 4.189.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀