open-amdocs / webrix

Powerful building blocks for React-based web applications
https://webrix.amdocs.com
Apache License 2.0
431 stars 31 forks source link

[Collapsible] refactor proposal #52

Closed yairEO closed 3 years ago

yairEO commented 3 years ago

I've noticed a the Collapsible documentation has duration in its props but it never actually uses it.

Moreover, the documentation states the duration's default value is 300, and after checking the source code, it does not appear to be so.

I then continued to look at the js source code of the Collapsible component, and thought it can be refactored, while also re-writing the whole CSS to use the duration prop while also allowing developers to set duration globally, like so:

.collapsible{ --duration:350ms }

It makes sense to allow developers the flexibility to define such things globally, so it wouldn't be needed to be applied to each <Collapsible> instance again and again, if some default wants to be changed.

My changes allow passing any time units as the duration prop, instead of only in ms

Before creating a PR, I've created a codesandbox


Refactor overview:

ykadosh commented 3 years ago

Looks awesome 👏 Can we get rid of the prop altogether then? We can just consume it through the CSS, and have some default value for it, like var(--duration, 300ms).

Also, I think we can get rid of .content-wrapper and just work with .content by taking the scrollHeight instead of the clientHeight, but that's a breaking change...

yairEO commented 3 years ago

Yes, I thought the same about .content-wrapper and also concluded it's a breaking change.

Yes, regarding the transition prop, I will drop it and prepare a PR for this and then I would need to add the proper documentation changes on the other docs repo. Also need to check which tests needs changing, if at all.

Happy you accept this refactor :)

ykadosh commented 3 years ago

Cool, sounds good 😁

udia76 commented 3 years ago

:tada: This issue has been resolved in version 1.3.4 :tada:

The release is available on:

Your semantic-release bot :package::rocket: