-
Notifications
You must be signed in to change notification settings - Fork 498
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
Start a new Thread by replying to a message #5089
Conversation
📱 Scan the QR code below to install the build for this PR. If you can't scan the QR code you can install the build via this link: https://i.diawi.com/oLCCoN |
…ults Thread search results
Thread Event Links
…tions Threaded message actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ismailgulek Thank you for providing a build containing all the Threads features you've been working on. I've reviewed it in detail and documented all the stuff that would need to be tweaked/improved. 👍
Please find all the details on the following annotated screenshots:
Implementation Review · P0 iOS · r01
https://www.figma.com/file/T309ztx0sNyOOK6NKVLHsK/Threads?node-id=2939%3A286198
Thanks!
Snapshot (partial)
@ismailgulek Quick update: I've added links to the mockups for the Thread List empty states. |
@ismailgulek Update: Added a note on applying the correct sizing rules for the Thread Summary component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff! 😎
I've added comments inline. One more general thought:
Should the new views be configuring the their labels using fonts from the theme rather than purely being set in the xibs/storyboards?
Riot/Modules/Threads/ThreadList/Views/Empty/ThreadListEmptyViewModel.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! Can't wait for this to land.
There's a few places where the variable/parameter is still called xyzViewModel
after the type was renamed to xyzModel
but that's only a minor thought.
Fix #5068 & #5069 & #5093 & #5266