ContextualMenu: Migrate existing ContextualMenu code to react-next #12992
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Asset size changes
Baseline commit: 039cf12124d228f15af2d7a77470a843d9149478 (build) |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
…t-copy/ContextualMenu
|
confused on the conflict |
|
@dzearing These conflicts happen whenever someone merges a change that copies code from OUFR, rather than importing it. |
…t-copy/ContextualMenu
…t-copy/ContextualMenu
|
I'm unclear on the failing focus tests, since this change has no code changes. :( |
ecraig12345
changed the title
ecraig12345
reviewed
May 13, 2020
| import { IComponentAs } from 'office-ui-fabric-react/lib/Utilities'; | ||
| import { IFocusZoneProps } from '@fluentui/react-focus'; | ||
| import { IFocusZoneProps } from 'office-ui-fabric-react/lib/FocusZone'; | ||
| import { IFocusZoneProps as IFocusZoneProps_2 } from '@fluentui/react-focus'; |
There was a problem hiding this comment.
Can you fix this to avoid exporting things twice? (same with IIconProps)
There was a problem hiding this comment.
I brought up the underlying issue in this comment
@dzearing @ecraig12345 Not sure of the advice concerning this build error:
@fluentui/react-next: [8:02:30 PM] x src/index.ts:18:1 - error TS2308: Module './Calendar' >has already exported a member named 'DateRangeType'. Consider explicitly re-exporting to >resolve the ambiguity. @fluentui/react-next: 18 export * from './DatePicker';Basically, since certain types are being re-exported from multiple components, and those types are now technically different when they're from different source files (OUFR vs react-next), we get this error.
Should we just copy OUFR over to react-next wholesale?
My question remains. I can't simply fix the duplication without either
- copying over a substantial portion (for this error) or all (for all similar errors) of OUFR over to react-next, or
- breaking existing export chains.
There was a problem hiding this comment.
Similar to what I responded on the other PR, I think the right thing to do for now is manually fixing up any duplicates (modifying import paths and removing duplicate exports as needed).
|
@MLoughry There were some changes to focus in popup and I cleaned up the focus tests in callout. I suspect that there might be some kind of pollution regarding focus when the tests were run. Especially if something like button was added to the page but never removed. I'll see if I can get some time to revamp the contextualmenu tests as well. |
|
Thanks for the updates! Looks like it's failing now due to missing snapshot updates. |
|
@ecraig12345 Those are the failing focus tests I referenced earlier. My understanding from @joschect's comment was that he was going to investigate and clean them up. |
…t-copy/ContextualMenu
|
@joschect Were you going to look into the focus issues? |
joschect
approved these changes
Jun 17, 2020
khmakoto
approved these changes
Jun 18, 2020
packages/react-next/src/components/SpinButton/SpinButton.types.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Makoto Morimoto <[email protected]>
Co-authored-by: Makoto Morimoto <[email protected]>
…t-copy/ContextualMenu
…t-copy/ContextualMenu
khmakoto
reviewed
Jun 22, 2020
| @@ -11,7 +11,7 @@ import { ContextualMenu } from './ContextualMenu'; | |||
| import { canAnyMenuItemsCheck } from './ContextualMenu.base'; | |||
| import { IContextualMenuItem, ContextualMenuItemType } from './ContextualMenu.types'; | |||
| import { IContextualMenuRenderItem, IContextualMenuItemStyles } from './ContextualMenuItem.types'; | |||
| import { DefaultButton, IButton } from '@fluentui/react-next/lib/Button'; | |||
| import { DefaultButton, IButton } from '@fluentui/react-next/lib/compat/Button'; | |||
There was a problem hiding this comment.
Suggested change
| import { DefaultButton, IButton } from '@fluentui/react-next/lib/compat/Button'; | |
| import { DefaultButton, IButton } from '../../compat/Button'; |
| @@ -4,7 +4,7 @@ import { IFocusZoneProps } from '../../FocusZone'; | |||
| import { IIconProps } from '@fluentui/react-next/lib/Icon'; | |||
| import { ICalloutProps, ICalloutContentStyleProps, Target } from '../../Callout'; | |||
| import { ITheme, IStyle } from '../../Styling'; | |||
| import { IButtonStyles } from '../../Button'; | |||
| import { IButtonStyles } from '@fluentui/react-next/lib/compat/Button'; | |||
There was a problem hiding this comment.
Suggested change
| import { IButtonStyles } from '@fluentui/react-next/lib/compat/Button'; | |
| import { IButtonStyles } from '../../compat/Button'; |
| @@ -3,7 +3,7 @@ import { IContextualMenuItem } from './ContextualMenu.types'; | |||
| import { IMenuItemClassNames } from './ContextualMenu.classNames'; | |||
| import { IStyle, ITheme } from '../../Styling'; | |||
| import { IRefObject, IStyleFunctionOrObject } from '../../Utilities'; | |||
| import { IButtonStyles } from '../../Button'; | |||
| import { IButtonStyles } from '@fluentui/react-next/lib/compat/Button'; | |||
There was a problem hiding this comment.
Suggested change
| import { IButtonStyles } from '@fluentui/react-next/lib/compat/Button'; | |
| import { IButtonStyles } from '../../compat/Button'; |
| @@ -1,11 +1,10 @@ | |||
| import * as React from 'react'; | |||
| import { Position } from 'office-ui-fabric-react/lib/utilities/positioning'; | |||
| import { IButtonStyles } from '@fluentui/react-next/lib/compat/Button'; | |||
| import { IButtonStyles, IButtonProps } from '@fluentui/react-next/lib/compat/Button'; | |||
There was a problem hiding this comment.
Suggested change
| import { IButtonStyles, IButtonProps } from '@fluentui/react-next/lib/compat/Button'; | |
| import { IButtonStyles, IButtonProps } from '../../compat/Button'; |
khmakoto
reviewed
Jun 22, 2020
| @@ -1,7 +1,7 @@ | |||
| import * as React from 'react'; | |||
| import { DirectionalHint } from '../../common/DirectionalHint'; | |||
| import { IFocusZoneProps } from '../../FocusZone'; | |||
| import { IIconProps } from 'office-ui-fabric-react/lib/Icon'; | |||
| import { IIconProps } from '@fluentui/react-next/lib/Icon'; | |||
There was a problem hiding this comment.
Suggested change
| import { IIconProps } from '@fluentui/react-next/lib/Icon'; | |
| import { IIconProps } from '../../Icon'; |
| @@ -15,6 +15,7 @@ import { | |||
| IContextualMenuItemStyleProps, | |||
| } from './ContextualMenuItem.types'; | |||
| import { IKeytipProps } from '../../Keytip'; | |||
| import { ICalloutContentStyles } from 'office-ui-fabric-react'; | |||
There was a problem hiding this comment.
Move this import as part of the imports in line 5.
| aria-describedby={mergeAriaAttributeValues(item.ariaDescription, keytipAttributes['aria-describedby'])} | ||
| aria-checked={item.isChecked || item.checked} | ||
| aria-describedby={mergeAriaAttributeValues( | ||
| item.ariaDescription as string, |
| item.ariaDescription as string, | ||
| keytipAttributes['aria-describedby'], | ||
| )} | ||
| aria-checked={(item.isChecked as boolean) || item.checked} |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request checklist
$ yarn changeDescription of changes
No code changes aside from fixing import paths
Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow