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 remaining components to forward their refs properly #4194

Closed
13 tasks done
bpas247 opened this issue Aug 7, 2019 · 8 comments
Closed
13 tasks done

Migrate remaining components to forward their refs properly #4194

bpas247 opened this issue Aug 7, 2019 · 8 comments

Comments

@bpas247
Copy link
Member

bpas247 commented Aug 7, 2019

Is your feature request related to a problem? Please describe

For examples on how to migrate these components, please read #4031, #4135, and #3992.

A clear and concise description of what the problem is.

Currently, not all of our components properly forward the passed in ref prop to the underlying component. This leads to end-users not being able to access the underlying DOM node (#3923 and #4012).

Describe the solution you'd like

Migrate the remaining class components to be ref forwarders. The components that need to be migrated are:
- [ ] Modal

Describe alternatives you've considered

So currently we have two different approaches of handling forwarding refs; per-component or through a HOC called createBootstrapComponent. I'm personally unsure about which approach to use for our codebase, but we should plan to choose one approach and stick with it for consistency. IMO we should handle this per-component, as using a HOC here might make the implementation of our codebase more complex than it's worth. This would involve migrating all of the components that use createBootstrapComponent, which some of them are included in the above list.

bpas247 added a commit that referenced this issue Aug 25, 2019
Migrates Image and ProgressBar components to forward their refs
properly.

Relevant to #4194.
bpas247 added a commit that referenced this issue Oct 5, 2019
@bpas247 bpas247 added the Hacktoberfest Issues during the month of October label Oct 8, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 10, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 10, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 10, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 10, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 10, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 12, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 12, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 12, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 12, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 12, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 12, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 12, 2019
jrwiegand pushed a commit to jrwiegand/react-bootstrap that referenced this issue Oct 13, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 14, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 14, 2019
zhuber added a commit to zhuber/react-bootstrap that referenced this issue Oct 14, 2019
jquense pushed a commit that referenced this issue Oct 14, 2019
* [#4194] - Page item forward ref

* [#4194] - Page item displayName and prettier cleanup
@hetpatel33
Copy link

hetpatel33 commented Oct 15, 2019

Took up DropdownToggle and the PR is #4690
@bpas247 @taion @mxschmitt please review.

@ankurkaushal
Copy link

Can I take on the Carousel component if it is still available?

@mxschmitt
Copy link
Member

Yep @ankurkaushal! Thank you :)

@bpas247 bpas247 removed the Hacktoberfest Issues during the month of October label Nov 7, 2019
@with-shrey
Copy link

@mxschmitt Any component remaining to be migrated ?

@taion
Copy link
Member

taion commented Jan 17, 2020

I removed Collapse and OverlayTrigger from the list above; they shouldn't actually forward, given that they're wrapping containers.

@bpas247
Copy link
Member Author

bpas247 commented Jan 17, 2020

Should we even migrate the Modal component? It just seems like a very costly migration, and I'm not sure if there's a lot of value we can get out of it.

@taion
Copy link
Member

taion commented Jan 18, 2020

hmm, arguably not... what would the ref even be to?

@bpas247
Copy link
Member Author

bpas247 commented Jan 27, 2020

hmm, arguably not... what would the ref even be to?

Good question, I'm not sure if it would be a reference to anything useful.

I'm going to go ahead and cross that out from the list, which would mark this issue as complete 🎉

Thank you to everyone who contributed their time for this major migration 😄 all of your continuing support is what makes this library as great as it is!

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

No branches or pull requests

6 participants