fix(tabs): redisplay close button when more than one tab is closable #11492
Conversation
…calcite-design-system into josercarcamo/10183-redisplay-tab-x
There was a problem hiding this comment.
This is looking good, @josercarcamo! One more pass, and I think we can land this.
We'll still need to handle the case where closed tab-titles are restored via the close prop being updated (i.e., items are closed via prop only). This can be done in a follow-up PR.
| * | ||
| * @private | ||
| */ | ||
| firstTabClosable = false; |
There was a problem hiding this comment.
Would renaming this private prop to something like lastClosableTabWasClosed help avoid the comment? Making the prop private could also remove the JSDoc tag if we need the comment.
There was a problem hiding this comment.
Maybe we should devise another name because lastClosableTabWasClosed is kind of long and not specific. Also, I only added to tag because other properties have tags. I can remove it and just make the property private.
There was a problem hiding this comment.
Sure, as long as it's specific and helps remove the comment. IMO, maintainability outweighs length due to the variable's scope, but I hear ya.
| * | ||
| * @private | ||
| */ | ||
| firstTabClosable = false; |
There was a problem hiding this comment.
This needs to be moved to the private props region.
| @@ -374,6 +381,10 @@ export class TabNav extends LitElement { | |||
| slottedElements.forEach((child) => { | |||
| this.intersectionObserver?.observe(child); | |||
| }); | |||
| if (slottedElements.length > 1 && this.firstTabClosable) { | |||
| (slottedElements[0] as TabTitle["el"]).closable = true; | |||
There was a problem hiding this comment.
I think this needs to check for closable=false to determine which tab-title should be updated instead of assuming it's the first one (e.g., if the first 4 out of 5 are closed, then a new tab and tab-title are added). Also, should firstTabClosable be reset here?
|
@jcfranco one more review please. |
There was a problem hiding this comment.
Had some additional comments, but this LGTM!
✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️
✖️📑✖️✖️✖️📑✖️✖️📑📑✖️✖️📑📑📑✖️✖️📑📑📑✖️📑📑📑📑✖️📑✖️
✖️📑📑✖️✖️📑✖️📑✖️✖️📑✖️✖️📑✖️✖️📑✖️✖️✖️✖️📑✖️✖️✖️✖️📑✖️
✖️📑✖️📑✖️📑✖️📑✖️✖️📑✖️✖️📑✖️✖️📑✖️✖️✖️✖️📑📑📑✖️✖️📑✖️
✖️📑✖️✖️📑📑✖️📑✖️✖️📑✖️✖️📑✖️✖️📑✖️✖️✖️✖️📑✖️✖️✖️✖️✖️✖️
✖️📑✖️✖️✖️📑✖️✖️📑📑✖️✖️📑📑📑✖️✖️📑📑📑✖️📑📑📑📑✖️📑✖️
✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️✖️
| @@ -370,10 +372,20 @@ export class TabNav extends LitElement { | |||
| private onSlotChange(event: Event): void { | |||
| this.intersectionObserver?.disconnect(); | |||
|
|
|||
| const slottedElements = slotChangeGetAssignedElements(event, "calcite-tab-title"); | |||
| const slottedElements = slotChangeGetAssignedElements<TabTitle["el"]>( | |||
There was a problem hiding this comment.
I think you could replace slottedElements with tabTitles directly to simplify.
| slottedElements.forEach((child) => { | ||
| this.intersectionObserver?.observe(child); | ||
| }); | ||
| const visibleTabTitlesIndices = this.visibleTabTitlesIndices; |
There was a problem hiding this comment.
You could reuse slottedElements/tabTitles and eliminate one extra DOM query by adding a function that returns the visible tab-title indices from an array of tab-titles.
| @@ -405,5 +405,24 @@ describe("calcite-tabs", () => { | |||
|
|
|||
| expect(selectedTitleOnEmit).toBe("Tab 2 Title"); | |||
| }); | |||
|
|
|||
| it("should hide x when last closable tab and display x when > 1 closable tabs", async () => { | |||
There was a problem hiding this comment.
For extra coverage, could you update or add a test for closing tabs 1–3? If adding a new test, could you group them in a describe block?
|
@josercarcamo FYI, I updated the PR title to better align with our PR naming conventions. |
|
This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions. |
…calcite-design-system into josercarcamo/10183-redisplay-tab-x
* origin/dev: (80 commits) build(deps): update nx monorepo to v20.4.6 (#11662) build(deps): update arcgis to ^4.32.7 (#11648) build: update browserslist db (#11639) build(deps): update eslint (#11670) build(deps): update dependency chromatic to v11.26.1 (#11669) build(deps): update dependency rollup to v4.34.9 (#11661) build(deps): update dependency tsx to v4.19.3 (#11658) chore: release next docs(text-area): update limitText description (#11672) feat(block, block-section): add `expanded` property and deprecate `open` property (#11582) feat(list-item, list): add `expanded` property and deprecate `open` property (#11003) chore: release next fix(tabs): redisplay close button when more than one tab is closable (#11492) test(shell-panel): Fix token story set up (#11635) chore: set up 3rd-party license generation (#11640) build(deps): update dependency happy-dom to v17.1.8 (#11649) build(deps): update dependency prettier to v3.5.2 (#11642) build(deps): update dependency happy-dom to v17.1.6 (#11641) chore: release next fix(card-group): Restore default gap spacing (#11638) ...
Related Issue: #10183
Summary
Redisplays x when > 1 closable tab is added programmatically.