Skip to content

Conversation

@venishpatidar
Copy link

What kind of change does this PR introduce?
This PR will introduces a new component Dropdown to RNE.

Did you add tests for your changes?
Yes all changes are tested.

If relevant, did you update the documentation?
Yes Documents have been updated

Summary
There will be new component to Dropdown to RNE which was long awaited request, and need not to be installed from other npm packages separately. Also this dropdown can be act as Accordion https://media.geeksforgeeks.org/wp-content/uploads/20190522140003/accordian.gif

Does this PR introduce a breaking change?
No

dropdown

@codecov
Copy link

codecov bot commented Apr 3, 2021

Codecov Report

Merging #2944 (c2315ec) into next (d78718c) will decrease coverage by 2.83%.
The diff coverage is 59.82%.

❗ Current head c2315ec differs from pull request most recent head 1d22ee3. Consider uploading reports for the commit 1d22ee3 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             next    #2944      +/-   ##
==========================================
- Coverage   88.20%   85.37%   -2.84%     
==========================================
  Files          50       57       +7     
  Lines        1009     1121     +112     
  Branches      399      441      +42     
==========================================
+ Hits          890      957      +67     
- Misses         76      105      +29     
- Partials       43       59      +16     
Impacted Files Coverage Δ
src/dropdown/DropdownChevron.tsx 23.07% <23.07%> (ø)
src/dropdown/Dropdown.tsx 48.57% <48.57%> (ø)
src/dropdown/DropdownItem.tsx 64.00% <64.00%> (ø)
src/dropdown/DropdownHead.tsx 76.00% <76.00%> (ø)
src/dropdown/DropdownSubtitle.tsx 80.00% <80.00%> (ø)
src/dropdown/DropdownTitle.tsx 80.00% <80.00%> (ø)
src/dropdown/DropdownContent.tsx 100.00% <100.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d78718c...1d22ee3. Read the comment docs.

@venishpatidar
Copy link
Author

venishpatidar commented Apr 3, 2021

@flyingcircle, I have upgraded it.

@flyingcircle
Copy link
Collaborator

Very nice! Yes, this is more like what I was thinking. I'm wondering though, it looks like you copied a good chunk of the code over. Why just use ListItem as is? Is there a compelling reason that We need DropdownItem and the associated components? Couldn't people just use ListItem?

@arpitBhalla
Copy link
Member

arpitBhalla commented Apr 4, 2021

I have a doubt, it is not called dropdown, It can be Expandable or Accordion

@venishpatidar
Copy link
Author

venishpatidar commented Apr 4, 2021

@flyingcircle yes, there is a reason, I was thinking as it would add more flexibility and rigidity. As further updated or modification to ListItem would not cause any bugs in Dropdown. Also other way it would be more flexible to be change or add anything to Dropdown in particular. Plus it matches the theme. Also using ListItem in Dropdown instead of Dropdown.Item will not cause dropdown to retract when onPress is defined in list.

@venishpatidar
Copy link
Author

@arpitBhalla Expandable is great suggestion.

@venishpatidar
Copy link
Author

venishpatidar commented Apr 9, 2021

@flyingcircle what's your opinion, should it need to be changed to Expandable or not.

@flyingcircle
Copy link
Collaborator

Decided to go with #2953 as it fit in a bit more with existing code.

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