msglist [nfc]: Convert the wrapper HOCs to more Hooks#5544
Merged
gnprice merged 8 commits intozulip:mainfrom Nov 11, 2022
Merged
msglist [nfc]: Convert the wrapper HOCs to more Hooks#5544gnprice merged 8 commits intozulip:mainfrom
gnprice merged 8 commits intozulip:mainfrom
Conversation
gnprice
added
a-message list
P1 high-priority
server release goal
chrisbobbe
approved these changes
Nov 11, 2022
Contributor
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
LGTM, thanks! Just one tiny comment comment below; please merge at will after seeing that.
Comment on lines
+67
to
+93
| /** | ||
| * All the data for rendering the message list, and callbacks for its UI actions. | ||
| * | ||
| * This data gets used for rendering the initial HTML, for computing | ||
| * inbound-events to update the page. Then the handlers for the message | ||
| * list's numerous UI actions -- both for user interactions inside the page | ||
| * as represented by outbound-events, and in action sheets -- use the data | ||
| * and callbacks in order to do their jobs. | ||
| * | ||
| * This is also -- hence the name -- the React props for the inner, function | ||
| * component, which include data obtained through the various HOCs below. | ||
| */ |
Contributor
There was a problem hiding this comment.
* This data gets used for rendering the initial HTML, for computing * inbound-events to update the page. […]
Should the comma be replaced with "and"? It looks like this might've started its life as "A, B, and C" but then you removed ", and C". Is that right? 😛
Member
Author
There was a problem hiding this comment.
Hmm yeah, indeed. I think C was what turned into the separate next sentence.
Member
Author
|
Thanks for the review! Fixing that nit and will merge. |
This "middle-props" notion will be temporary for within this series of refactoring commits, to manage having a mix of some items provided by HOCs and others by Hooks.
This originally described the props supplied by our callback passed to `connect`. Now that everything's just one big function component, that's no longer such a meaningful distinction.
gnprice
force-pushed
the
pr-msglist-hooks-more
branch
from
20a9886 to
8383a3e
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This follows up on #5524 by replacing the various props-providing higher-order components we have wrapping this component --
connect,connectActionSheet,withGetText-- with just more hooks.MessageListis now simply a function component, rather than a series of wrappers around an inner function component.As a bonus, we clean up a pair of calls to
assumeSecretlyGlobalState.Like #5524, this is on the road to #5364.