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: add prop right to List Accordion #2531

Merged
merged 1 commit into from
Apr 9, 2021
Merged

Conversation

Joeao
Copy link
Contributor

@Joeao Joeao commented Jan 29, 2021

Summary

This pull request addresses issue #2150. It gives users the flexibility to replace what's rendered on the right-hand-side of the accordion with their own component. The expanded state is passed back to the user so that they can render based on this value.

This feature draws inspiration from the left prop on the same component. Instead, this could have been addressed similarly to how Fab Icon is implemented, however the drawback is that the implementation of Fab Icon is a bit more complex when compared to the left prop of this component.

Test plan

Render a List.Accordion component and pass in a separate component as the right prop. E.g.

Omitting this prop results in the default MaterialCommunityIcon element being rendered.

@callstack-bot
Copy link

Hey @Joeao, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

@sobiamaredia
Copy link

Can someone please merge this PR? This is a great addition.

@lukewalczak
Copy link
Member

lukewalczak commented Apr 8, 2021

Hey @Joeao, I understand the purpose of that PR, however I'm afraid it's against principles. I was not able to find the clarification on MaterialDesign documentation whether the icon on the right in ListAccordion can be customizable and can be different than chevron. Could you please point me to the documentation to confirm that it follows the design rules?

@Joeao
Copy link
Contributor Author

Joeao commented Apr 9, 2021

Hi @lukewalczak.

In my case it's not that I want to use an icon other than Chevron up/down, it's that I want to avoid using MaterialCommunityIcon.

The intention of this PR isn't to go against Material Design principals, but to allow for more flexibility in icon library choices. I could have mentioned that in my initial comment but I hadn't been thinking about principals. I now see that the issue I referenced suggests going against the principals, for that I condemn it and refuse to have this PR be associated with it any longer!

I know that the RNP documentation says that some components may render incorrectly if not using the default icons, but it'd be nice to be able to reduce that as much as possible which this PR would do in one case.

As to whether or not RNP would like to tackle the issue of icon replacement either in a different way or not at all is up to you and the Callstack team. If this PR is a suboptimal way to address the problem right now then feel free to close. I can say that my project would benefit from it as currently there is no Chevron, just an empty box, which definitely goes against the principals. But my individual case doesn't mean much against the 60+ thousand weekly download users who expect a robust, well supported component library that follows the principals that they know and love.

@lukewalczak
Copy link
Member

lukewalczak commented Apr 9, 2021

Hey @Joeao thanks for your comment. I understand your point of view and your intention. I think since the right prop default is always chevron from MaterialCommunityIcon we can accept that PR and introduce the prop to allow more flexibility in that matter.

@lukewalczak lukewalczak merged commit e309e2e into callstack:main Apr 9, 2021
@Joeao
Copy link
Contributor Author

Joeao commented Apr 9, 2021

Excellent, thank you very much!

@dslandry
Copy link

It seems like this feature is not present in the latest 4.7.2 version. I've installed it in my project but the prop seems to be missing

@lukewalczak
Copy link
Member

It's not released yet, in the meantime you can use package from main branch.

@dslandry
Copy link

Please do you know when it will be released?

@lukewalczak
Copy link
Member

Hopefully until the end of the week 🤞🏽

@lukewalczak
Copy link
Member

New version is already release (4.8.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants