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

Fix AccordionHeader toggleExpanded #1121

Merged
merged 1 commit into from
Feb 22, 2021
Merged

Fix AccordionHeader toggleExpanded #1121

merged 1 commit into from
Feb 22, 2021

Conversation

cbuv
Copy link
Contributor

@cbuv cbuv commented Feb 19, 2021

problem:
AccordionItem shadows external toggleExpanded. The internal and external props are mixed.
A common use case is to defer render of components that are not visible to the user. I many of the inhouse application do have a lot of data and hidden components may be expensive to render.

solution:
add a new onToggle prop to support toggleExpanded when it's wrapped in AccordionItem.

#1120

@cbuv cbuv changed the title Fix AccordionHeader toggleExpanded (#1120) Fix AccordionHeader toggleExpanded Feb 19, 2021
@cbuv
Copy link
Contributor Author

cbuv commented Feb 19, 2021

what's the secret to make the react-tests build pass?
all tests are passing.

@wenche
Copy link
Contributor

wenche commented Feb 19, 2021

what's the secret to make the react-tests build pass?
all tests are passing.

Seems like an eslint issue, do you run eslint and prettier locally? @cbuv

Skjermbilde 2021-02-19 kl  13 53 24

@mimarz
Copy link
Contributor

mimarz commented Feb 19, 2021

what's the secret to make the react-tests build pass?
all tests are passing.

Its the magic comma! 🙈

Don't worry, we are switching to github actions at this exact moment so it'll be more transparent in the future :)

@wenche
Copy link
Contributor

wenche commented Feb 19, 2021

We will add Husky for precommit hooks in another project we are involved in - could probably be useful for EDS as well. Sorry about that!

@cbuv
Copy link
Contributor Author

cbuv commented Feb 19, 2021

thanks, I actually did pnpm run lint:all first, but didnt notice the 401. and then I couldn't access the logs.
And I'm used to prettier default config. 🤣

Copy link
Contributor

@mimarz mimarz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks for your contribution in making EDS better! 👏

@mimarz mimarz merged commit b6d85fd into equinor:develop Feb 22, 2021
@cbuv cbuv deleted the accordion-header-ontoggle branch March 24, 2021 16:55
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.

3 participants