Skip to content
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

Chore/heading and button components #192

Merged
merged 22 commits into from Mar 12, 2019
Merged

Conversation

ryami333
Copy link
Contributor

Splices the 'button' element out of AccordionItemHeading and into its own AccordionItemButton component.

A nice side-effect of this refactor was that the additional 'arrow' div was able to be refactored into a pseudo-element of the button, and as such the user no longer needs to add it explicitly.

@@ -17,10 +18,6 @@ import './main.css';
// tslint:disable-next-line no-import-side-effect ordered-imports
import '../../src/css/fancy-example.css';

const Arrow = (): JSX.Element => (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still arrows, but they're no longer explicit. Check out fancy-example.css and you'll see that they're now just a pseudo-element on accordion__button.

@@ -39,240 +54,272 @@ describe('WAI ARIA Spec', () => {
expect(title).toBe(
'React Accessible Accordion - Integration Test Sandbox',
);

await browser.close();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above - I now do this on afterEach instead (prevents memory leaks when tests fail).

it('When focus is on the accordion header for a collapsed panel, expands the associated panel. If the implementation allows only one panel to be expanded, and if another panel is expanded, collapses that panel.', async () => {
const { browser, page, headingsHandles } = await setup();
expect(headingsHandles.length).toEqual(3);
it(`When focus is on the accordion header for a collapsed panel,
Copy link
Contributor Author

@ryami333 ryami333 Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored all the test strings to be <80cols (but strings haven't actually changed)


const firstHeadingHandle = headingsHandles[0];
const secondHeadingHandle = headingsHandles[1];
const firstButtonHandle = buttonsHandles[0];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using buttons now in most places instead of headings, because that's what the aria-spec actually calls for.

Copy link
Contributor

@samanthaksanders samanthaksanders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! Can't really spot any issues

README.md Outdated

### AccordionItemButton

#### className : `string` [*optional*, default: `'accordion__heading'`]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think you maybe forgot to update the class here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted, cheers.

README.md Outdated

#### className : `string` [*optional*, default: `'accordion__heading'`]

Class(es) to apply to the 'heading' element.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, 'button' not heading :)

@ryami333 ryami333 merged commit cb2db5e into next Mar 12, 2019
@ryami333 ryami333 deleted the chore/heading-and-button-components branch March 12, 2019 20:18
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 87.435% when pulling 80dd4cd on chore/heading-and-button-components into a99abcf on next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants