SlotFill: several code cleanups #50098
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsnajdr
added
[Type] Code Quality
|
Size Change: -89 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Apr 26, 2023
| } | ||
|
|
||
| const Fill = ( props ) => ( |
There was a problem hiding this comment.
I wonder why we had this in the first place.
There was a problem hiding this comment.
FillComponent used to be a class component, wrapped in a context consumer component, and then it was refactored to functional with hooks in #15541. But the PR kept the context consumer wrapper, it didn't migrate it to useContext.
tyxla
approved these changes
Apr 26, 2023
That CI check is optional, and this PR doesn't make any user observable changes. The changelog entry can hardly say anything more than "code cleanup, removed unused code" and that's not meaningful to the library user. That's why I'll merge without a changelog entry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While learning about how
SlotFillexactly works, I found some unused code or cleanup opportunities, and I'm submitting them here.Fillcomponent (the non-bubbling) version was composed from a functionalFillComponentwrapped inside aSlotFillContext.Consumercomponent. By using theuseContext( SlotFillContext )hook, we can merge the entire functionality into one functional component.SlotFillProvidercomponent has ahasFillsmethod that's not used anywhere, and the context is also not exposed as public API. It can be removed.SlotandFillcomponents have unused code -- thebindNodemethod and acreatePortalcall -- that was used only in thebubblesVirtuallyversion which was extracted into separate components in Components: Refactor SlotFill #19242. It can be removed completely from the non-bubbling version.useSlotcan be written as oneexport fromcall. Just like the adjacent export ofuseSlotFills.How to test:
Check that all unit and e2e tests are passing after changing these widely used components.