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

feat(react-tree): TreeItem itemType restructure #27799

Conversation

bsunderhus
Copy link
Contributor

@bsunderhus bsunderhus commented May 9, 2023

Previous Behavior

The TreeItem expects a certain positioning of the internal children to ensure the presence of a subtree

New Behavior

This PR ensures that TreeItem is agnostic regarding it's children content by:

  1. introducing itemType property to make it the users responsibility to declare if a TreeItem is either a leaf or a branch
  2. moves slots definitions (expandIcon and actions were moved) from TreeItem to TreeItemLayout and TreeItemPersonaLayout to follow up with TreeItem component lack of ability to differ from layout content and subtree content.
  3. makes subtree Tree declaration responsible for it's own visibility, instead of TreeItem
  4. starts using CSS grid display on TreeItem to ensure actions and aside slots will occupy the same visual space without the requirement of using any sort of absolute positioning

@fabricteam
Copy link
Collaborator

fabricteam commented May 9, 2023

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 707 714 5000
Button mount 367 370 5000
Field mount 1269 1284 5000
FluentProvider mount 919 891 5000
FluentProviderWithTheme mount 114 122 10
FluentProviderWithTheme virtual-rerender 98 106 10
FluentProviderWithTheme virtual-rerender-with-unmount 115 111 10
InfoButton mount 19 21 5000
MakeStyles mount 1121 1111 50000
Persona mount 2067 2057 5000
SpinButton mount 1597 1569 5000

@size-auditor
Copy link

size-auditor bot commented May 9, 2023

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 27f252e27361df75a94c5bc5b09fc0097fefd969 (build)

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 9, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b4cd954:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@fabricteam
Copy link
Collaborator

fabricteam commented May 9, 2023

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Button, FluentProvider & webLightTheme
64.855 kB
17.852 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
203.885 kB
57.081 kB
react-components
react-components: FluentProvider & webLightTheme
36.086 kB
11.9 kB
react-portal-compat
PortalCompatProvider
6.446 kB
2.186 kB
🤖 This report was generated against 27f252e27361df75a94c5bc5b09fc0097fefd969

@bsunderhus bsunderhus force-pushed the react-tree/feat--TreeItem-itemType-restructure branch from 8a60998 to daf8076 Compare May 9, 2023 14:10
@bsunderhus bsunderhus marked this pull request as ready for review May 10, 2023 08:29
@bsunderhus bsunderhus requested a review from a team as a code owner May 10, 2023 08:29
@bsunderhus bsunderhus force-pushed the react-tree/feat--TreeItem-itemType-restructure branch from daf8076 to de1fa29 Compare May 10, 2023 11:26
@bsunderhus bsunderhus force-pushed the react-tree/feat--TreeItem-itemType-restructure branch from de1fa29 to b4cd954 Compare May 10, 2023 11:44
@bsunderhus bsunderhus merged commit e61227d into microsoft:master May 10, 2023
@bsunderhus bsunderhus deleted the react-tree/feat--TreeItem-itemType-restructure branch May 10, 2023 12:16
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request May 10, 2023
* master:
  feat(react-drawer): create DrawerBody component (microsoft#27581)
  feat(react-tree): TreeItem itemType restructure (microsoft#27799)
  feat: Implement state management for toasts (microsoft#27800)
  bugfix: fix VisibleFlatTreeItemGenerator omitting visible items (microsoft#27802)
  fix: overflowManager should always dispatch initial state (microsoft#27756)
  fix(react-badge): Remove white border around presence badge when on a dark background (microsoft#27780)
  react-tags: rename dismissable to dismissible (microsoft#27798)
  chore: update ownership of react-tags (microsoft#27795)
  applying package updates
  feat(react-tags): Replace `avatar` prop with `media`; polish styles for spacing (microsoft#27725)
  RFC appearance migration (microsoft#24181)
  chore(react-skeleton): Release react-skeleton to stable (microsoft#27767)
  fix(Coachmark): Remove positioning regression and update bounds on resize (microsoft#27782)
  applying package updates
  fix(v8): explicitly publish dist folder after node 16 upgrade (microsoft#27769)
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request May 10, 2023
* feat/drawer-header:
  fix: add changefiles
  feat(react-drawer): create DrawerBody component (microsoft#27581)
  feat(react-tree): TreeItem itemType restructure (microsoft#27799)
  feat: Implement state management for toasts (microsoft#27800)
  bugfix: fix VisibleFlatTreeItemGenerator omitting visible items (microsoft#27802)
  fix: overflowManager should always dispatch initial state (microsoft#27756)
  fix(react-badge): Remove white border around presence badge when on a dark background (microsoft#27780)
  react-tags: rename dismissable to dismissible (microsoft#27798)
  chore: update ownership of react-tags (microsoft#27795)
  applying package updates
  feat(react-tags): Replace `avatar` prop with `media`; polish styles for spacing (microsoft#27725)
  RFC appearance migration (microsoft#24181)
  chore(react-skeleton): Release react-skeleton to stable (microsoft#27767)
  fix(Coachmark): Remove positioning regression and update bounds on resize (microsoft#27782)
  applying package updates
  fix(v8): explicitly publish dist folder after node 16 upgrade (microsoft#27769)
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request May 10, 2023
* master: (29 commits)
  feat(react-drawer): create DrawerBody component (microsoft#27581)
  feat(react-tree): TreeItem itemType restructure (microsoft#27799)
  feat: Implement state management for toasts (microsoft#27800)
  bugfix: fix VisibleFlatTreeItemGenerator omitting visible items (microsoft#27802)
  fix: overflowManager should always dispatch initial state (microsoft#27756)
  fix(react-badge): Remove white border around presence badge when on a dark background (microsoft#27780)
  react-tags: rename dismissable to dismissible (microsoft#27798)
  chore: update ownership of react-tags (microsoft#27795)
  applying package updates
  feat(react-tags): Replace `avatar` prop with `media`; polish styles for spacing (microsoft#27725)
  RFC appearance migration (microsoft#24181)
  chore(react-skeleton): Release react-skeleton to stable (microsoft#27767)
  fix(Coachmark): Remove positioning regression and update bounds on resize (microsoft#27782)
  applying package updates
  fix(v8): explicitly publish dist folder after node 16 upgrade (microsoft#27769)
  applying package updates
  fix: do not use outlineStyle: none in createCustomFocusIndicatorStyle (microsoft#26089)
  fix: Scale pulse animation with percentages and flip wave animation's direction (microsoft#27654)
  Publish dist folder that got removed due to node 16 upgrade (microsoft#27764)
  fix(scripts-update-release-notes): properly handle git for-each-ref cmd call to not fail release notes update (microsoft#27757)
  ...
@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-tree@v9.0.0-beta.13 has been released which incorporates this pull request.:tada:

Handy links:

});

export const useTreeStyles_unstable = (state: TreeState): TreeState => {
const styles = useStyles();
state.root.className = mergeClasses(treeClassNames.root, styles.root, state.root.className);

const isSubtree = state.level > 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: as you may need to apply styles via re-composition, it might be useful to defined isSubtree under state

Comment on lines +7 to +11
const open = useTreeItemContext_unstable(ctx => ctx.open);

const { dir } = useFluent_unstable();

const expandIconRotation = open ? 90 : dir !== 'rtl' ? 0 : 180;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spacing is a bit strange there, also would invert a condition 🐱

  const open = useTreeItemContext_unstable(ctx => ctx.open);
  const { dir } = useFluent_unstable();

  const expandIconRotation = open ? 90 : dir === 'rtl' ? 180 : 0;

@@ -1,20 +1,23 @@
import { ButtonContextValue } from '@fluentui/react-button';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import type { ButtonContextValue } from '@fluentui/react-button';

nit:

{slots.iconBefore && <slots.iconBefore {...slotProps.iconBefore} />}
{slotProps.root.children}
{slots.iconAfter && <slots.iconAfter {...slotProps.iconAfter} />}
{!isActionsVisible && slots.aside && <slots.aside {...slotProps.aside} />}
<ButtonContextProvider value={state.buttonContextValue}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you may want to render a provider conditionally

{state.isActionsVisible && slots.actions && (
  <ButtonContextProvider value={state.buttonContextValue}>
    <slots.actions {...slotProps.actions} />
  </ButtonContextProvider>
)}

Or even:

{state.isActionsVisible && slots.actions && (
  <slots.actions {...slotProps.actions}>
    <ButtonContextProvider value={state.buttonContextValue}>
      {slotProps.actions.children}
    </ButtonContextProvider>
  </slots.actions {...slotProps.actions}>
)}

import type { TreeItemLayoutProps, TreeItemLayoutState } from './TreeItemLayout.types';
import { useTreeItemContext_unstable } from '../../contexts/treeItemContext';
import { TreeItemChevron } from '../TreeItemChevron';
import { ButtonContextValue } from '@fluentui/react-button';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import type { ButtonContextValue } from '@fluentui/react-button';

nit

@@ -1,33 +1,36 @@
import type { ComponentProps, ComponentState, Slot } from '@fluentui/react-utilities';
import type { AvatarContextValue, AvatarSize } from '@fluentui/react-avatar';
import type { TreeItemContextValue } from '../../contexts/treeItemContext';
import { ButtonContextValue } from '@fluentui/react-button';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import type { ButtonContextValue } from '@fluentui/react-button';

nit

<TreeItem expandIcon="">
<TreeItemPersonaLayout description="Secondary text content" media={<Avatar />}>
<TreeItem itemType="leaf">
<TreeItemPersonaLayout expandIcon="" description="Secondary text content" media={<Avatar />}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should expandIcon={null} be to avoid slot rendering?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants