Mobile notifications #3137
Mobile notifications #3137
Conversation
Generated by 🚫 dangerJS |
|
Hey @mxstbr, this is getting to be really big, so let's merge it asap to avoid conflicts with other mobile branches. There's still a lot of jank and bugs abound, but the notifications view now looks good! You'll also see how I'm constructing some utility components for lists: 41ebac8 Those components are now sitting underneath threads, direct messages, and notifications. There might be a bit more refactoring to do in there, but so far it's really nice to develop with (much better than how we approached lists on web :P) I also ported over our icon file, but it has some different implementation than on web since react native SVG behaves a bit different: 7729264 |
|
For context, here's a before/after too on DMs!
|
|
Cleaned up most of the nitpicks, let's discuss about the others before merging! |
|
|
||
| class WithCurrentUser extends React.Component<Props> { | ||
| render() { | ||
| return this.props.render({ currentUser: this.props.data.user }); |
There was a problem hiding this comment.
Overall this feels pretty great from reading the code, how'd it feel writing it?
Nitpick: let's make this accept either children or render: this.props.children ? this.props.children({}) : this.props.render({})
What kind of jank? Do you have some more specific words for me? |
List items collapsing, but I think this is partly because of my |
|
Ohh I've seen that collapsing in the messages! Try clicking on a shortened quote in a message to open it up, and for some reason some other messages collapse into nothingness—it's super weird! |
|
429cea5 improves the <CurrentUser>
{({ currentUser }) => (
// ...
)}
</CurrentUser>or <CurrentUser render={({ currentUser }) => (
// ...
)} />In addition, there's a proper hoc for All that being said - I'm still seeing some list items collapsing in the DM inbox, notifications, messages, threads, etc. That seems like a separate issue for us to debug though, huH? |
|
@mxstbr if you pull this latest branch can you see if your DM threads collapse? This seems to be a new bug introduced overnight or by me this morning, because they didn't collapse last night. |
mxstbr
left a comment
mxstbr
left a comment
There was a problem hiding this comment.
Super nice, great work, this is awesome and gets us much closer to shippability! 👏
Wanna open an issue about the collapsing bug? It's always happened, it has nothing to do with this PR. I thought it was a simulator thing, I haven't experienced in on device yet, but I obviously can't run on device since Androids broken.
mxstbr
left a comment
mxstbr
left a comment
There was a problem hiding this comment.
Just gotta fix Flow and Peril, apparently
I'm experiencing it on-device as well - happens a lot when anything is paginated, too. It's super weird. I'll open another issue now. |
|
Flow is passing for me locally :/ This PR might have gotten too big for Peril to work |
|
Not talking about peril, the Circle |
|
@mxstbr any ideas on that snapshot failure in test_web? super weird |
|
Ah fuck that's my bad I forgot to update that. I added an |
Status
Deploy after merge (delete what needn't be deployed)
cc @mxstbr to discuss how we want to approach redux +
currentUserconcept. Proof of concept in b7b58b5