-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(react-card): add focusMode prop
#22312
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
Changes from all commits
b311e06
90ffeeb
8d5cb1e
543d0e0
1054f5a
9a5e04b
8fe5c64
80d6c7a
3b2b94a
160574c
e90464b
bd89c2a
c222356
bf2970a
b5f7f26
500b150
2d55a67
a2804ba
d192755
be9688a
bc54b30
1efb5f4
7427282
77b469d
e6a3cd3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "prerelease", | ||
| "comment": "Added new `focusMode` property to control the focus behavior inside of the component", | ||
| "packageName": "@fluentui/react-card", | ||
| "email": "[email protected]", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,243 @@ | ||
| import * as React from 'react'; | ||
| import { mount } from '@cypress/react'; | ||
| import type {} from '@cypress/react'; | ||
| import { FluentProvider } from '@fluentui/react-provider'; | ||
| import { webLightTheme } from '@fluentui/react-theme'; | ||
| import { Button } from '@fluentui/react-button'; | ||
| import { Card, CardFooter, CardHeader } from '@fluentui/react-card'; | ||
| import type { CardProps } from '@fluentui/react-card'; | ||
|
|
||
| const mountFluent = (element: JSX.Element) => { | ||
| mount(<FluentProvider theme={webLightTheme}>{element}</FluentProvider>); | ||
| }; | ||
|
|
||
| const CardSample = (props: CardProps) => { | ||
| const ASSET_URL = 'https://raw.githubusercontent.com/microsoft/fluentui/master/packages/react-card'; | ||
|
|
||
| const powerpointLogoURL = ASSET_URL + '/assets/powerpoint_logo.svg'; | ||
|
|
||
| return ( | ||
| <> | ||
| <p tabIndex={0} id="before"> | ||
| Before | ||
| </p> | ||
| <Card id="card" {...props}> | ||
| <CardHeader | ||
| image={<img src={powerpointLogoURL} alt="Microsoft PowerPoint logo" />} | ||
| header={<b>App Name</b>} | ||
| description={<span>Developer</span>} | ||
| /> | ||
| <div> | ||
| Donut chocolate bar oat cake. Dragée tiramisu lollipop bear claw. Marshmallow pastry jujubes toffee sugar | ||
| plum. | ||
| </div> | ||
| <CardFooter> | ||
| <Button id="open-button" onClick={alert} appearance="primary"> | ||
| Open | ||
| </Button> | ||
| <Button id="close-button" appearance="outline"> | ||
| Close | ||
| </Button> | ||
| </CardFooter> | ||
| </Card> | ||
| <p tabIndex={0} id="after"> | ||
| After | ||
| </p> | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| describe('Card', () => { | ||
| describe('focus behaviors', () => { | ||
| describe('focusMode="off" (default)', () => { | ||
| it('should not be focusable', () => { | ||
| mountFluent(<CardSample />); | ||
|
|
||
| cy.get('#before').focus(); | ||
|
|
||
| cy.get('#card').should('not.be.focused'); | ||
|
|
||
| cy.realPress('Tab'); | ||
|
|
||
| cy.get('#card').should('not.be.focused'); | ||
| cy.get('#open-button').should('be.focused'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('focusMode="no-tab"', () => { | ||
| it('should be focusable', () => { | ||
| mountFluent(<CardSample focusMode="no-tab" />); | ||
|
|
||
| const card = cy.get('#card'); | ||
|
|
||
| card.should('not.be.focused'); | ||
|
|
||
| card.focus(); | ||
|
|
||
| card.should('be.focused'); | ||
| }); | ||
|
|
||
| it('should focus inner elements on EnterKey press', () => { | ||
| mountFluent(<CardSample focusMode="no-tab" />); | ||
|
|
||
| cy.get('#card').focus(); | ||
|
|
||
| cy.realPress('Enter'); | ||
|
|
||
| cy.get('#open-button').should('be.focused'); | ||
| }); | ||
|
|
||
| it('should not focus inner elements on Tab press', () => { | ||
| mountFluent(<CardSample focusMode="no-tab" />); | ||
|
|
||
| cy.get('#card').focus(); | ||
|
|
||
| cy.realPress('Tab'); | ||
|
|
||
| cy.get('#card').should('not.be.focused'); | ||
| cy.get('#after').should('be.focused'); | ||
| }); | ||
|
|
||
| it('should trap focus', () => { | ||
| mountFluent(<CardSample focusMode="no-tab" />); | ||
|
|
||
| cy.get('#open-button').focus(); | ||
|
|
||
| cy.realPress('Tab'); | ||
|
|
||
| cy.get('#open-button').should('not.be.focused'); | ||
| cy.get('#close-button').should('be.focused'); | ||
|
|
||
| cy.realPress('Tab'); | ||
|
|
||
| cy.get('#open-button').should('be.focused'); | ||
| cy.get('#close-button').should('not.be.focused'); | ||
| }); | ||
|
|
||
| it('should focus parent on Esc press', () => { | ||
| mountFluent(<CardSample focusMode="no-tab" />); | ||
|
|
||
| cy.get('#open-button').focus(); | ||
|
|
||
| cy.realPress('Escape'); | ||
|
|
||
| cy.get('#open-button').should('not.be.focused'); | ||
| cy.get('#card').should('be.focused'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('focusMode="tab-exit"', () => { | ||
| it('should be focusable', () => { | ||
| mountFluent(<CardSample focusMode="tab-exit" />); | ||
|
|
||
| const card = cy.get('#card'); | ||
|
|
||
| card.should('not.be.focused'); | ||
|
|
||
| card.focus(); | ||
|
|
||
| card.should('be.focused'); | ||
| }); | ||
|
Comment on lines
+130
to
+140
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: the focusable card test could be in another test suite. You could use Sometimes I also think duping tests can be nice because reusable tests are always a bit less readable. So up to you if you want to do this or not
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer to go with verbosity over efficiency for tests. Tests should be as readable as possible overall and the less "magic" we add to them, the better.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the ancient wisdom says: "it depends" :D |
||
|
|
||
| it('should focus inner elements on EnterKey press', () => { | ||
| mountFluent(<CardSample focusMode="tab-exit" />); | ||
|
|
||
| cy.get('#card').focus(); | ||
|
|
||
| cy.realPress('Enter'); | ||
|
|
||
| cy.get('#open-button').should('be.focused'); | ||
| }); | ||
|
|
||
| it('should not focus inner elements on Tab press', () => { | ||
| mountFluent(<CardSample focusMode="tab-exit" />); | ||
|
|
||
| cy.get('#card').focus(); | ||
|
|
||
| cy.realPress('Tab'); | ||
|
|
||
| cy.get('#card').should('not.be.focused'); | ||
| cy.get('#after').should('be.focused'); | ||
| }); | ||
|
|
||
| it('should exit on Tab press', () => { | ||
| mountFluent(<CardSample focusMode="tab-exit" />); | ||
|
|
||
| cy.get('#close-button').focus(); | ||
|
|
||
| cy.realPress('Tab'); | ||
|
|
||
| cy.get('#after').should('be.focused'); | ||
| }); | ||
|
|
||
| it('should focus parent on Esc press', () => { | ||
| mountFluent(<CardSample focusMode="tab-exit" />); | ||
|
|
||
| cy.get('#card').focus().realPress('Enter'); | ||
|
|
||
| cy.get('#open-button').should('be.focused'); | ||
|
|
||
| cy.realPress('Escape'); | ||
|
|
||
| cy.get('#card').should('be.focused'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('focusMode="tab-only"', () => { | ||
| it('should be focusable', () => { | ||
| mountFluent(<CardSample focusMode="tab-only" />); | ||
|
|
||
| const card = cy.get('#card'); | ||
|
|
||
| card.should('not.be.focused'); | ||
|
|
||
| card.focus(); | ||
|
|
||
| card.should('be.focused'); | ||
| }); | ||
|
|
||
| it('should focus inner elements on EnterKey press', () => { | ||
| mountFluent(<CardSample focusMode="tab-only" />); | ||
|
|
||
| cy.get('#card').focus(); | ||
|
|
||
| cy.realPress('Enter'); | ||
|
|
||
| cy.get('#open-button').should('be.focused'); | ||
| }); | ||
|
|
||
| it('should focus inner elements on Tab press', () => { | ||
| mountFluent(<CardSample focusMode="tab-only" />); | ||
|
|
||
| cy.get('#card').focus(); | ||
|
|
||
| cy.realPress('Tab'); | ||
|
|
||
| cy.get('#card').should('not.be.focused'); | ||
| cy.get('#open-button').should('be.focused'); | ||
| }); | ||
|
|
||
| it('should exit on Tab press', () => { | ||
| mountFluent(<CardSample focusMode="tab-only" />); | ||
|
|
||
| cy.get('#close-button').focus(); | ||
|
|
||
| cy.realPress('Tab'); | ||
|
|
||
| cy.get('#after').should('be.focused'); | ||
| }); | ||
|
|
||
| it('should focus parent on Esc press', () => { | ||
| mountFluent(<CardSample focusMode="tab-only" />); | ||
|
|
||
| cy.get('#card').focus().realPress('Enter'); | ||
|
|
||
| cy.get('#open-button').should('be.focused'); | ||
|
|
||
| cy.realPress('Escape'); | ||
|
|
||
| cy.get('#card').should('be.focused'); | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "extends": "../tsconfig.json", | ||
| "compilerOptions": { | ||
| "isolatedModules": false, | ||
| "noEmit": true, | ||
| "types": ["node", "cypress", "cypress-storybook/cypress", "cypress-real-events"], | ||
| "lib": ["ES2019", "dom"] | ||
| }, | ||
| "include": ["**/*.ts", "**/*.tsx"] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ export const cardClassNames: SlotClassNames<CardSlots>; | |
| // @public (undocumented) | ||
| export type CardCommons = { | ||
| appearance: 'filled' | 'filled-alternative' | 'outline' | 'subtle'; | ||
| focusMode: 'off' | 'no-tab' | 'tab-exit' | 'tab-only'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙌 |
||
| }; | ||
|
|
||
| // @public | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you seem to use these selectors multiple times, would be worth hoisting them to variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The duplication is intentional for readability purposes as hardcoded values makes the test clearer (less need to hop around to understand what it is doing).