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

feat(PPDSC-2152): single accordion #224

Merged
merged 22 commits into from
Jun 16, 2022
Merged

Conversation

Xin00163
Copy link
Contributor

@Xin00163 Xin00163 commented Jun 7, 2022

PPDSC-2152

  • Build the single Accordion interface
  • Add storybook scenarios
  • Add unit tests

I have done:

  • Written unit tests against changes
  • Written functional tests against the component and/or NewsKit site
  • Updated relevant documentation

I have tested manually:

  • The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
  • The screen reader reads and flows through the elements as expected.
  • There are no new errors in the browser console coming from this PR.
  • When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
  • The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

@Xin00163 Xin00163 marked this pull request as ready for review June 7, 2022 07:47
@newskit-engineering
Copy link
Collaborator

@GeriReid
Copy link
Contributor

GeriReid commented Jun 7, 2022

@Xin00163 run through - looking good.

A couple of text colour changes:

  • Disabled: start enhancer icon, label and chevron icon - color: inkNonEssential
  • On hover: start enhancer icon, chevron icon and label - color: interativeInput40
    My error, I should have specced these more clearly, I thought they’d be covered by font colour but I should have been more specific. Mockup here.

Divider/bottom border. When the accordion panel is open can the bottom border of the header be removed so you don’t get a double border?

src/accordion/__tests__/accordion.stories.tsx Outdated Show resolved Hide resolved
src/accordion/styled.ts Outdated Show resolved Hide resolved
src/accordion/styled.ts Show resolved Hide resolved
src/accordion/accordion.tsx Outdated Show resolved Hide resolved
src/accordion/styled.ts Outdated Show resolved Hide resolved
src/accordion/accordion.tsx Outdated Show resolved Hide resolved
src/accordion/styled.ts Outdated Show resolved Hide resolved
src/accordion/styled.ts Outdated Show resolved Hide resolved
@Xin00163
Copy link
Contributor Author

Xin00163 commented Jun 13, 2022

@Xin00163 run through - looking good.

A couple of text colour changes:

  • Disabled: start enhancer icon, label and chevron icon - color: inkNonEssential
  • On hover: start enhancer icon, chevron icon and label - color: interativeInput40
    My error, I should have specced these more clearly, I thought they’d be covered by font colour but I should have been more specific. Mockup here.

Divider/bottom border. When the accordion panel is open can the bottom border of the header be removed so you don’t get a double border?

Regarding the above, I've managed to line up the state stylePresets of header text and label but since startEnhancer and indicatorIcon cannot inherit the parent stylePreset, I've not managed to have them behave the same way as label.

A few things have changed since last review and our discussion:

  • Divider has been removed and we will use borderBottom instead.
  • Header is kept as optional because there is a case user might have a label but not header. The modal component's header is optional as well.
  • onFocus state will be added after https://nidigitalsolutions.jira.com/browse/PPDSC-2151 is merged.
  • default stylePresets have also been removed for indicatorLabel & indicatorIcon

@Xin00163 Xin00163 added the ready for review Please assist in getting this reviewed label Jun 13, 2022
src/accordion/accordion.tsx Outdated Show resolved Hide resolved
src/accordion/types.ts Outdated Show resolved Hide resolved
src/accordion/accordion.tsx Outdated Show resolved Hide resolved
src/accordion/accordion.tsx Outdated Show resolved Hide resolved
src/accordion/styled.ts Outdated Show resolved Hide resolved
src/accordion/styled.ts Show resolved Hide resolved
src/accordion/__tests__/accordion.stories.tsx Outdated Show resolved Hide resolved
src/accordion/__tests__/accordion.stories.tsx Outdated Show resolved Hide resolved
src/accordion/__tests__/accordion.stories.tsx Show resolved Hide resolved
src/accordion/types.ts Outdated Show resolved Hide resolved
src/accordion/__tests__/accordion.stories.tsx Outdated Show resolved Hide resolved
src/accordion/__tests__/accordion.stories.tsx Show resolved Hide resolved
src/accordion/accordion.tsx Show resolved Hide resolved
src/accordion/__tests__/accordion.test.tsx Outdated Show resolved Hide resolved
src/accordion/__tests__/accordion.test.tsx Outdated Show resolved Hide resolved
src/accordion/__tests__/accordion.test.tsx Show resolved Hide resolved
Copy link
Contributor Author

@Xin00163 Xin00163 left a comment

Choose a reason for hiding this comment

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

All comments should have been addressed. Really appreciate getting all these feedback

@mutebg mutebg merged commit 08ad3ac into main Jun 16, 2022
@mutebg mutebg deleted the feat/PPDSC-2152-single-accordion branch June 16, 2022 09:02
Xin00163 added a commit that referenced this pull request Oct 17, 2022
* fix(PPDSC-2152): wip

* fix(PPDSC-2152): wip

* fix(PPDSC-2152): wip

* fix(PPDSC-2152): update component structure

* feat(PPDSC-2152): added disabled state

* feat(PPDSC-2152): a11y

* fix(PPDSC-2152): overrides scenario

* fix(PPDSC-2152): unit tests

* feat(PPDSC-2152): fix:overrides test

* fix(PPDSC-2152): e2e tests

* fix(PPDSC-2152): clean up

* fix(PPDSC-2152): address comments

* fix(PPDSC-2152): update storybook

* fix(PPDSC-2152): address reviews

* fix(PPDSC-2152): address more comments

* fix(PPDSC-2152): remove leftover

* fix(PPDSC-2152): address more reviews

Co-authored-by: agagotowiec <[email protected]>
Co-authored-by: Stoyan Delev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Please assist in getting this reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants