FocusZone and FocusTrapZone example improvements #8505
Conversation
ecraig12345
force-pushed
the
focus-examples
branch
from
18c06ea to
4364964
Compare
|
Size Auditor did not detect a change in bundle size for any component! |
| @@ -44,11 +44,9 @@ export default class BoxExample extends React.Component<React.HTMLAttributes<HTM | |||
| const { isChecked } = this.state; | |||
|
|
|||
| return ( | |||
| <div className="ms-FocusTrapZoneBoxExample"> | |||
| <div className={contentClass}> | |||
There was a problem hiding this comment.
Is there a reason not to use a Stack here? Seems to be a need for a container with styling, which sounds like a Stack case to me. Since this is an example I'd rather steer people towards Stack than div with mergeStyles usage. (This comment applies to multiple examples in this PR.)
There was a problem hiding this comment.
Good point. I made them all use Stacks (and look nicer). While doing that and looking at the examples, I realized that the toggles in most of the examples were totally redundant with the buttons, so I took them out. Let me know if you disagree with that.
There was a problem hiding this comment.
The only concern I have with removing the Toggle is that it's less obvious that there is a FocusTrapZone active preventing interaction with the rest of the page...
There was a problem hiding this comment.
Hmm interesting. Maybe I could remove the buttons instead? It just seems wrong to have multiple ways of entering/exiting the trap zone...
There was a problem hiding this comment.
That seems fair. I think the Toggles make it a little more clear when an FTZ is active and preventing you from interacting with the rest of the component page:
| <div className="ms-Row"> | ||
| <FocusZone direction={FocusZoneDirection.horizontal}> | ||
| export const FocusZoneDisabledExample: React.StatelessComponent = () => ( | ||
| <Stack gap={20} horizontalAlign="start"> |
There was a problem hiding this comment.
I'm hoping we can get #8247 in sooner than later rather than continue to sprinkle gap in our examples which I think it a styling anti-pattern. (I'm also curious if warnDeprecations on gap will cause build issues with all of these as well.)
There was a problem hiding this comment.
Yeah @khmakoto will have to do a find/replace of gap before checking in his PR. An alternative to prevent further delays to the big PR would be omitting the warnDeprecations call for now, then doing a separate PR to add the warning and fix all instances.
There was a problem hiding this comment.
I'm working on getting the changes so we can check that PR. Hopefully I'll get that done today.
|
Closing and reopening to trigger new PR job on Dev Ops. |
|
I like the new look! 👍
|
ecraig12345
force-pushed
the
focus-examples
branch
from
1353790 to
005be27
Compare
ecraig12345
force-pushed
the
focus-examples
branch
from
005be27 to
e947e2e
Compare
|
@JasonGore could you take another look at this PR? |
|
🎉 Handy links: |
Pull request checklist
$ npm run changeDescription of changes
Convert the FocusZone and FocusTrapZone examples to use css-in-js rather than SCSS so they can be exported to CodePen. I also made some improvements to the styling of the examples (with the magic of Stacks!) and did various other code cleanup.
Microsoft Reviewers: Open in CodeFlow