Coachmark: Migrate Coachmark and associated components to function components #13626
Conversation
Co-authored-by: Elizabeth Craig <ecraig12345@gmail.com>
…fabric-react into functional/Coachmark
There was a problem hiding this comment.
I don't see any issues with this PR. To be transparent, I'm not the original codeowner of the previous implementation; John Miller built most of the core functionality, and I took over after he left. As per @ecraig12345 's suggestion, I'm more than happy to jump on a call if we want to discuss anything in detail, but this LGTM.
|
@ecraig12345 @xugao Could either of you approve the Screener additions? |
|
It looks like someone rejected the "Coachmark: Expanded: Default" for current Coachmark because it's not actually showing the expanded coachmark. And in the CoachmarkNext expanded state, I'm not seeing the beak.
|
|
Actually never mind, I don't see a beak on the example on the website either, so maybe the CoachmarkNext one is okay. But I still think the Coachmark current expanded is wrong. |
|
@ecraig12345 I replied to @xugao about that very issue:
|
|
Oh sorry, I missed that. Maybe we should remove the screener state for now so we're not checking in an incorrect state? |
|
I accepted the CoachmarkNext expanded state. |
|
|
||
| // @TODO rename to reflect the name of this class | ||
| private _contentHost = React.createRef<HTMLDivElement>(); | ||
| function useTargets({ target }: IPositioningContainerProps, positionedHost: React.RefObject<HTMLDivElement | null>) { |
There was a problem hiding this comment.
This is optional, but since PositioningContainer is a fork of Callout, would it be possible to reuse any of the hooks from Callout here? Seems like a pretty easy way to actually start taking advantage of the potential hooks provide for sharing logic.
There was a problem hiding this comment.
@ecraig12345 I'll do this in a PR fixing ContextualMenu's UNSAFE methods. Since this change has a package.json change that causes conflicts after every publish, I'd like to merge this today.
|
🎉 Handy links: |
…mponents (microsoft#13626) * Copy Coachmark code to react-next to stage migration * Change files * Migrate Beak to function component * Add stub function component for PositioningContainer * Hoist default props and contentHost ref * Hoist several refs * Fix lint issues * Hoist event logic * Update API * Cleanup * Hoist heightOffset logic * Finish migration (with issues) * Revert to LKG, fixing example import * Hoist positionedHost ref * Hoist target refs * Hoist positions state * Hoist setInitialFocus * Hoist maxHeight * Hoist auto-dismiss events * Hoist heightOffset * Complete PositioningContainer migration * Remove extraneous shouldComponentUpdate * Hoist generic function * Create stub function component * Hoist entityInnerHostElementRef * Hoist isCollapsed state * Hoist positioned data * Hoist beak positioning logic * Hoist dismiss public method * Hoist document event logic * Hoist proximity handlers * Hoist aria text logic * Cleanup unused state * Hoist autofocus logic * Hoist last of state * Finish migration * Forward ref * Update API * Change files * Add Screener test * Fix lint error * Fix cropping of Coachmark story * Fix case where Coahmark starts expanded * Fix API file * Pull from react-next * Fix bad merge * Duplicate Coachamrk Screener test for OUFR * Remove headers * Handle removed props * Deprecate Beak componentRef * Update packages/react-next/src/components/Coachmark/Coachmark.base.tsx Co-authored-by: Elizabeth Craig <ecraig12345@gmail.com> * Fix eslint deprecation errors * Change files * Update API * Fix build error * Fix build issues * Remove rejected screener story Co-authored-by: Elizabeth Craig <ecraig12345@gmail.com>
Pull request checklist
$ yarn changeDescription of changes
This change migrates
Coachmarkand its associated componentsBeakandPositioningContainerto function components in thereact-nextpackage.Focus areas to test
(optional)