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

Added partition for controlled overflow menu #27520

Conversation

ValentinaKozlova
Copy link
Contributor

@ValentinaKozlova ValentinaKozlova commented Apr 11, 2023

  • Added partition for controlled overflow menu
  • Added unit tests
  • Used Overflow component
  • Styling will be done in the following PR

New Behavior

Behavior with Overflow component:
Note: last item should be always visible

Animation

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 11, 2023

📊 Bundle size report

🤖 This report was generated against 4acd563e14d4a257585cff2dba2fb826ce9c059b

@size-auditor
Copy link

size-auditor bot commented Apr 11, 2023

Asset size changes

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

Baseline commit: 4acd563e14d4a257585cff2dba2fb826ce9c059b (build)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 11, 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 2577fca:

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

@ValentinaKozlova ValentinaKozlova marked this pull request as ready for review April 14, 2023 18:14
@ValentinaKozlova ValentinaKozlova requested a review from a team as a code owner April 14, 2023 18:14
@ValentinaKozlova ValentinaKozlova force-pushed the feat/breadcrumb-overflow-menu branch 2 times, most recently from b991769 to 4a6b1fa Compare April 19, 2023 16:56
@ling1726
Copy link
Member

ling1726 commented Apr 24, 2023

This does not behave as it should, take a look at the recording
overflow broken

There are a few things things causing the available space calculation to mess up. I just went with something like below as a quick workaround, but I think some of these underlying issues can be fixed in the overflow package

<Overflow padding={40} />

Style reset for <ol>

The <ol> tag is shipped with default user agent styles, you should reset those styles the padding and margins are messing up the available space. You could probably use the padding prop in the Overflow component too, but I don't think you want those styles anyway.

image

Divider width

I think this is rather overlooked from me. The overflow library was designed with the vertical dividers in mind that didn't take up that much space, which is essentially what toolbars used. A simple padding in the Overflow component could just solve this, but it might be worth looking into more complex dividers for that library.

image

Unmanaged divider

You have a divider unmanaged by the overflow library, which also throws off available space calculations.

          <ControlledOverflowMenu
            overflowItems={overflowItems}
            startDisplayedItems={startDisplayedItems}
            endDisplayedItems={endDisplayedItems}
          />
          <BreadcrumbDivider />

@ValentinaKozlova
Copy link
Contributor Author

This does not behave as it should, take a look at the recording overflow broken overflow broken

There are a few things things causing the available space calculation to mess up. I just went with something like below as a quick workaround, but I think some of these underlying issues can be fixed in the overflow package

<Overflow padding={40} />

Style reset for <ol>

The <ol> tag is shipped with default user agent styles, you should reset those styles the padding and margins are messing up the available space. You could probably use the padding prop in the Overflow component too, but I don't think you want those styles anyway.

image

Divider width

I think this is rather overlooked from me. The overflow library was designed with the vertical dividers in mind that didn't take up that much space, which is essentially what toolbars used. A simple padding in the Overflow component could just solve this, but it might be worth looking into more complex dividers for that library.

image

Unmanaged divider

You have a divider unmanaged by the overflow library, which also throws off available space calculations.

          <ControlledOverflowMenu
            overflowItems={overflowItems}
            startDisplayedItems={startDisplayedItems}
            endDisplayedItems={endDisplayedItems}
          />
          <BreadcrumbDivider />

Hi @ling1726 , I did style reset in a separate PR. Hope, I'll merge it tomorrow. And I talked to Tudor about the divider issue. I can collaborate on fixing it when the major features of Breadcrumb are ready. There are accessibility and focus left

@ValentinaKozlova ValentinaKozlova mentioned this pull request Apr 24, 2023
28 tasks
@ValentinaKozlova ValentinaKozlova merged commit e9c67ec into microsoft:master Apr 28, 2023
22 checks passed
@ValentinaKozlova ValentinaKozlova deleted the feat/breadcrumb-overflow-menu branch April 28, 2023 11:01
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request May 2, 2023
* master:
  applying package updates
  fix: dont ignore lib, lib-commonjs and lib-amd files (microsoft#27736)
  Add DraggableZone into export map (microsoft#27569)
  Add export for FolderCover.scss (microsoft#27507)
  applying package updates
  Add filetype icon for Microsoft Designer (microsoft#27724)
  ci: add build task back as lint pre-requirement to unblock temporarily import plugin lint rule (microsoft#27729)
  fix(react-charting): add missing dependency to fix lint violations during release (microsoft#27728)
  Added partition for controlled overflow menu (microsoft#27520)
  Azure Theme 8.0: Tag Button variant for DefaultButton and PrimaryButton (microsoft#27186)
  Azure Theme 8.0 dropdown fix for high contrast themes (microsoft#27346)
  chore: enable TS intellisense in cross library projects for better/consistent DX (microsoft#26605)
  chore: enforce files naming to use .styles.ts [cxe-red files] (microsoft#27710)
  chore(v0): enable emit only dts and use new conformance test API to narrow down TS Program (microsoft#27686)
  codeowner update (microsoft#27719)
  Tag/TagButton init component setup (microsoft#27102)
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request May 2, 2023
* feat/drawer-components: (120 commits)
  fix: add missing change file
  Component testing - Donut chart (microsoft#27033)
  fix: update .npmignore files to resolve npm8/node16 regression with npm publish for v9 packages (microsoft#27741)
  chore: add .swcrc to .npmignore (generator) (microsoft#27739)
  docs: update API
  Autogenerate react-toast package (microsoft#27730)
  applying package updates
  fix: dont ignore lib, lib-commonjs and lib-amd files (microsoft#27736)
  Add DraggableZone into export map (microsoft#27569)
  Add export for FolderCover.scss (microsoft#27507)
  applying package updates
  Add filetype icon for Microsoft Designer (microsoft#27724)
  ci: add build task back as lint pre-requirement to unblock temporarily import plugin lint rule (microsoft#27729)
  fix(react-charting): add missing dependency to fix lint violations during release (microsoft#27728)
  Added partition for controlled overflow menu (microsoft#27520)
  Azure Theme 8.0: Tag Button variant for DefaultButton and PrimaryButton (microsoft#27186)
  Azure Theme 8.0 dropdown fix for high contrast themes (microsoft#27346)
  chore: enable TS intellisense in cross library projects for better/consistent DX (microsoft#26605)
  chore: enforce files naming to use .styles.ts [cxe-red files] (microsoft#27710)
  chore(v0): enable emit only dts and use new conformance test API to narrow down TS Program (microsoft#27686)
  ...
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.

None yet

4 participants