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

Feature/accordion component #813

Merged

Conversation

j212122
Copy link
Contributor

@j212122 j212122 commented Jun 23, 2023

Summary of the changes

Creates accordion and accordion-group.

Related issue

#240

Deployed:
https://j212122.github.io/ic-ui-kit/branches/feature/accordion-component/web-components/?path=/story/web-components-accordion--default-story

Checklist

  • I have added relevant unit and visual regression tests.
  • I have manually accessibility tested any changes, if relevant.
  • I have ensured any changes match the Figma component library.

@j212122 j212122 force-pushed the feature/accordion-component branch 2 times, most recently from 4e1d2ec to 4ebc9f4 Compare June 27, 2023 13:55
@gd2910
Copy link
Contributor

gd2910 commented Jun 28, 2023

New components should adhere to the Stencil style guide;
Tests in a 'test' sub-directory
Each test in its own semantically named directory

Component files should also follow this pattern;
Variables (alphabetised)
Element
State (alphabetised)
Prop (alphabetised)
Watch (with watched props behind them)
Event (alphabetised)
Lifecycle (folowing lfiecycle process: connection, load, update, render)
Listen
Method
Local private methods
Render

Copy link
Contributor

@GCHQ-Developer-530 GCHQ-Developer-530 left a comment

Choose a reason for hiding this comment

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

Looks really good, just a couple of small comments 😊

@j212122 j212122 force-pushed the feature/accordion-component branch 6 times, most recently from be397a8 to 7e5e2ab Compare June 29, 2023 09:53
@jd239 jd239 changed the base branch from develop to a11y/accordion June 29, 2023 10:37
Copy link
Contributor

@GCHQ-Developer-530 GCHQ-Developer-530 left a comment

Choose a reason for hiding this comment

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

Found a small bug where if you click see all, but then manually close the accordions, it still says hide all even though they are all hidden.

Screen.Recording.2023-06-29.at.13.43.36.mov

@j212122 j212122 force-pushed the feature/accordion-component branch 12 times, most recently from 586ca30 to ed2e8fa Compare July 4, 2023 13:22
@j212122 j212122 force-pushed the feature/accordion-component branch from ed2e8fa to 1d9b938 Compare July 7, 2023 09:56
@j212122 j212122 marked this pull request as draft July 7, 2023 09:56
@j212122 j212122 force-pushed the feature/accordion-component branch 2 times, most recently from 00ec242 to 45fbfec Compare July 7, 2023 12:13
Copy link
Contributor

@GCHQ-Developer-530 GCHQ-Developer-530 left a comment

Choose a reason for hiding this comment

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

Spoke to designers and the borders should use the border default variable set up in #735. Other than that, looks great!

@j212122 j212122 force-pushed the feature/accordion-component branch 3 times, most recently from 970d593 to 78b2356 Compare July 18, 2023 09:45
Copy link
Contributor

@GCHQ-Developer-847 GCHQ-Developer-847 left a comment

Choose a reason for hiding this comment

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

Added a few comments - everything else looks great though!

@j212122 j212122 force-pushed the feature/accordion-component branch from 78b2356 to f40abc4 Compare July 18, 2023 16:33
@j212122 j212122 marked this pull request as draft July 19, 2023 15:08
@j212122 j212122 force-pushed the feature/accordion-component branch 5 times, most recently from d323fa6 to c739f7c Compare July 24, 2023 10:11
@j212122 j212122 marked this pull request as ready for review July 24, 2023 10:49
@gd2910
Copy link
Contributor

gd2910 commented Jul 24, 2023

Just a small one, looks like the background colour for 'light text' and 'parent' stories are mixed up.

@gd2910
Copy link
Contributor

gd2910 commented Jul 24, 2023

There are a couple of private variables that can be brought up to the top of the class in ic-accordion.tsx

 private animationTimer: ReturnType<typeof setTimeout>;
  private expandedContentEl: HTMLDivElement;

@j212122 j212122 force-pushed the feature/accordion-component branch 2 times, most recently from be3ef3f to e6d2231 Compare July 25, 2023 10:07
Copy link
Contributor

@GCHQ-Developer-530 GCHQ-Developer-530 left a comment

Choose a reason for hiding this comment

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

The disabled accordion seems to still have a click state, I think the behaviour should be that it shouldn't change background colour when clicked.

Screen.Recording.2023-07-26.at.13.34.56.mov

@j212122 j212122 force-pushed the feature/accordion-component branch 4 times, most recently from d099f58 to fbe3996 Compare July 28, 2023 08:03
@j212122 j212122 force-pushed the feature/accordion-component branch from fbe3996 to c4a33a5 Compare July 28, 2023 08:14
@GCHQ-Developer-530 GCHQ-Developer-530 merged commit 719c665 into mi6:a11y/accordion Jul 28, 2023
4 checks passed
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.

5 participants