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

Scrollable tabs #2440

Merged
merged 61 commits into from
Oct 23, 2024
Merged

Scrollable tabs #2440

merged 61 commits into from
Oct 23, 2024

Conversation

atmgrifter00
Copy link
Contributor

@atmgrifter00 atmgrifter00 commented Oct 16, 2024

Pull Request

🤨 Rationale

👩‍💻 Implementation

Adding buttons to template which will show/hide based on width of the container for the tabs either exceeding visible area or not. Layout of tabs is now using flex instead of grid (the original use of grid wasn't clear to me).

ResizeObserver vs IntersectionObserver

I currently have opted to use a ResizeObserver for controlling when to show/hide the scroll buttons. The reason for this is two-fold:

  1. The observer only fires when the contentRect for the element containing the tabs changes. This will occur if the Tabs component changes its width, when tabs or added or removed (if all tabs are visible), or when the content of a tab is changed (when all tabs are visible), all of which are valid conditions to re-evaluate whether we need to show or hide the scroll buttons.
  2. The implementation is easy to reason about, and there is no extra state necessary.

Two different approaches were used/attempted with the IntersectionObserver. The first involved observing each Tab element. While this worked, the implementation was cumbersome and harder to reason about, as it involved extra state that the Tabs component had to maintain (a map that tracked which tabs were currently visible or not), and "calculating" the visible condition from the state kept in the map. This was required to handle cases such as removing the last tab, where it was only partially visible (and thus was currently showing the scroll buttons), which, when removed, the IntersectionObserver would only report a change for the removed tab, but the visibility should still be updated.

The other approach attempted was trying to simply observe the container element of the tabs (whose root was simply its containing element), which wouldn't have required any extra state. However, this requires the observer to fire both when the container element becomes partially visible and when it becomes fully visible. Supplying thresholds of [0, 1] (one pixel and 100% visible), or only 0, did not result in the needed behavior. It would simply fire once, and then never again.

🧪 Testing

Added unit tests, and matrix tests.

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@atmgrifter00 atmgrifter00 requested a review from m-akinc October 16, 2024 15:20
packages/nimble-components/src/tabs/template.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/tabs/template.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/tabs/styles.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/tabs/styles.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/tabs/index.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/tabs/tests/tabs.spec.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/tabs/tests/tabs.spec.ts Outdated Show resolved Hide resolved
@atmgrifter00 atmgrifter00 requested a review from m-akinc October 17, 2024 18:13
@m-akinc m-akinc marked this pull request as ready for review October 17, 2024 21:40
@atmgrifter00 atmgrifter00 requested a review from rajsite October 23, 2024 14:41
@atmgrifter00 atmgrifter00 requested a review from jattasNI October 23, 2024 14:42
@atmgrifter00 atmgrifter00 merged commit 4173c31 into main Oct 23, 2024
14 checks passed
@atmgrifter00 atmgrifter00 deleted the scrollable-tabs branch October 23, 2024 19:04
rajsite pushed a commit that referenced this pull request Oct 29, 2024
# Pull Request

## 🤨 Rationale

The update to the `Tabs` component introducing scroll buttons #2440 also
updated the `display` setting to `flex`. This resulted in a missed
change in behavior with how the `TabPanel` might size to the height of
the `Tabs` component. See [this
discussion](https://dev.azure.com/ni/DevCentral/_git/Skyline/pullrequest/821815?discussionId=7029412).

## 👩‍💻 Implementation

Just needed to set a style on `[part="tabpanel"]` to have `flex: 1;`.

## 🧪 Testing

Added a Chromatic test that would fail without the style change.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.
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

Successfully merging this pull request may close these issues.

5 participants