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

Migrate collapse package to Typescript #644

Merged

Conversation

arnaudforaison
Copy link
Contributor

Description of the issue

To have all toolkit's packages written in Typescript, this PR migrate collapse package

Important

  • Class component are rewritten in functional component
  • Component use hooks
  • Others expects are made for CollapseCardBase because instance doesn't work with React 16.x and functional component

xballoy
xballoy previously approved these changes Jul 7, 2020
Copy link
Contributor

@xballoy xballoy left a comment

Choose a reason for hiding this comment

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

LGTM

@arnaudforaison arnaudforaison force-pushed the feature/migrate-collapse-ts branch from af27f1d to 4388fc1 Compare September 3, 2020 08:13
@arnaudforaison
Copy link
Contributor Author

Need some reviewers


describe('Accordion', () => {
it('handleToggle should return correct value', () => {
let oldCollapses = [false];
Copy link
Contributor

Choose a reason for hiding this comment

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

const instead of let?

Copy link
Contributor

@xballoy xballoy left a comment

Choose a reason for hiding this comment

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

LGTM.
Can you rebase with master and push again?

@arnaudforaison arnaudforaison merged commit 2c004c6 into AxaFrance:master Oct 16, 2020
@arnaudforaison arnaudforaison deleted the feature/migrate-collapse-ts branch February 17, 2021 16:49
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.

6 participants