Remove KeytipData usage from Link, Checkbox, Toggle (react-next) #13742
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
xugao
commented
Jun 22, 2020
| <Toggle onText="Yes" offText="No" keytipData={toggleKeytipData} /> | ||
| <span> | ||
| Go to{' '} | ||
| <Link keytipData={linkKeytipData} href="http://www.bing.com" target="_blank"> |
There was a problem hiding this comment.
Before:
<Link keytipProps={keytipMap.LinkKeytip} href="http://www.bing.com" target="_blank">
After:
const linkKeytipData = useKeytipData({ keytipProps: keytipMap.LinkKeytip });
<Link keytipData={linkKeytipData} href="http://www.bing.com" target="_blank">
There was a problem hiding this comment.
Instead why not this?
<Keytip {...keytipProps}>
<Link href="..." target="...">...</Link>
</Keytip>There was a problem hiding this comment.
Updated the code:
Before:
<Link keytipProps={keytipMap.LinkKeytip} href="http://www.bing.com" target="_blank">
After:
const linkRef = useKeytipRef({ keytipProps: keytipMap.LinkKeytip });
<Link ref={linkRef} href="http://www.bing.com" target="_blank">
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Asset size changes
Baseline commit: d43db14983c012854c144263821109e09ed15310 (build) |
khmakoto
reviewed
Jun 22, 2020
change/@fluentui-react-next-2020-06-22-11-18-19-xgao-useKeytipData.json
Outdated
Show resolved
Hide resolved
change/office-ui-fabric-react-2020-06-22-11-18-19-xgao-useKeytipData.json
Outdated
Show resolved
Hide resolved
packages/office-ui-fabric-react/src/components/KeytipData/useKeytipData.ts
Outdated
Show resolved
Hide resolved
xugao
force-pushed
the
xgao/useKeytipData
branch
3 times, most recently
from
fe87fa5
to
656b064
Compare
|
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 10d068e:
|
kelseyyoung
reviewed
Jun 25, 2020
kelseyyoung
reviewed
Jun 25, 2020
xugao
force-pushed
the
xgao/useKeytipData
branch
from
656b064
to
0c74b56
Compare
xugao
force-pushed
the
xgao/useKeytipData
branch
2 times, most recently
from
1ade513
to
2cba581
Compare
|
@dzearing @khmakoto @kelseyyoung - I updated the code and added The idea is that the hook will return a ref which then user can pass the ref to whichever component keytip is targeting. The ref will allow the hook to access the root element that it's targeting and sets proper keytip attributes based on (optional) data attributes set within the root element. |
|
@xugao would the ref approach be problematic if the 'ref' attribute for the component was already used? |
|
@kelseyyoung - user can merge multiple refs into one. we have an implementation in |
dzearing
reviewed
Jul 9, 2020
| * Optional keytip for this Link | ||
| * Optional keytip. | ||
| * | ||
| * @deprecated This no longer works. Use `useKeytipData` hook instead. |
There was a problem hiding this comment.
This doesn't seem like the right deprecation message - is there any way to have this value deprecated in v7 with an alternative implementation for customers?
There was a problem hiding this comment.
If we can deprecate it (but keep it functional)_ with the viable alternative in v7, and then remove it in v8, that's a better user experience. They can update to wrapping things before the upgrade, right now, in v7.
There was a problem hiding this comment.
Unfortunately user can't use the new approach on V7 components, because they don't support passing ref to the root element
dzearing
reviewed
Jul 9, 2020
dzearing
reviewed
Jul 9, 2020
dzearing
reviewed
Jul 9, 2020
packages/react-next/src/components/Toggle/__snapshots__/Toggle.test.tsx.snap
Show resolved
Hide resolved
dzearing
approved these changes
Jul 9, 2020
xugao
force-pushed
the
xgao/useKeytipData
branch
from
64e5189
to
10d068e
Compare
|
🎉 Handy links: |
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
Removed
KeytipDatausage from Link, Checkbox, Toggle components:keytipPropsuseKeytipRefhook and pass the ref to the targeted componentHow to apply
Keytipto a component after this change, here is an example:You can also find usage examples in
Keytips.Basic.Example.tsxFocus areas to test
(optional)