improved layout of verification modal#950
improved layout of verification modal#950jenniferhmartinezmejia-netizen wants to merge 3 commits into
Conversation
kevinaboos
left a comment
kevinaboos
left a comment
There was a problem hiding this comment.
Hi @jenniferhmartinezmejia-netizen, thanks for your contribution!
I left some comments below. While things generally look fine, I get the sense that you basically just copy-pasted Element's design decisions into Robrix. Please don't just directly copy Element; we're not trying to make an Element clone here, and I don't want to get accused of IP theft.
The only thing we need to actually change here in Robrix is the emoji layout. Nothing else -- the button order, messages shown in the modal body, text phrasing, etc doesn't need to change.
Please disclose what AI Agents or LLMs you used to generate this PR, and whether you manually reviewed each line. If not, please do so as part of your next commit.
|
Hey Kevin! Thank you for reviewing the changes closely. I will make sure to review the comments and make the changes later this week when I have time. I didn't mean to copy Element, apologies for that. I was just trying to follow the comments made by matlu on the issue. That's my bad for not double-checking if you wanted those changes or not. Have a nice rest of your day! |
|
all good, thanks! |
kevinaboos
left a comment
kevinaboos
left a comment
There was a problem hiding this comment.
thanks for addressing most of the issues I raised. Code is generally looking good, but I left a few more questions for new things that I noticed.
| // The emoji grid is only relevant during the `KeysExchanged` emoji | ||
| // step; hide it by default and let that branch re-show it, so it | ||
| // never leaks into other states. |
There was a problem hiding this comment.
please don't use LLM-style comments. This can be really simple.
| // The emoji grid is only relevant during the `KeysExchanged` emoji | |
| // step; hide it by default and let that branch re-show it, so it | |
| // never leaks into other states. | |
| // The emoji grid is hidden by default and only shown during emoji verification. |
| ); | ||
| for (index, emoji) in emoji_list.emojis.iter().enumerate() { | ||
| self.populate_emoji_cell(cx, index, emoji.symbol, emoji.description); | ||
| } |
There was a problem hiding this comment.
this is better than the previous C-style loop that you had before, but we can do even better than this with Rust. Check out the .zip() iterator function, which will let you iterate over the emoji list and the set of emoji cells in lockstep, which itself will let you avoid the match index { ... } block in populate_emoji_cell().
Once you see how that works, you'll recognize that you can also easily move the body of populate_emoji_cell() (which is really just the two lines that set the symbol and description labels) directly into this code block.
| // Ensure the emoji grid from any prior verification is not shown | ||
| // on the initial prompt screen. |
There was a problem hiding this comment.
again, LLM comments like this aren't necessary, you can just delete it entirely. Good comments explain a design choice or why we're doing something complex that is non-obvious, but here it is very obvious what's happening.
| // Ensure the emoji grid from any prior verification is not shown | |
| // on the initial prompt screen. |
| } | ||
| } | ||
| description := Label { | ||
| width: Fit{max: FitBound.Abs(72.0)}, height: Fit |
There was a problem hiding this comment.
hmm, i don't quite understand this width choice. Can you explain why we need a max width here? Seems like for something like this we would want both the symbol and the description to have a fixed width (perhaps 72 as you used for the max width here).
I can't think of a scenario where we'd need a width that is Fit but expands to a max of 72. But let me know if i'm overlooking something.
kevinaboos
added
the
waiting-on-author
2 participants
Note: a PR template is required for the CodePath class I am taking. I couldn't find a template in the repo so I used the one provided in the class.
What does this PR do?
Replaces the plain text list of SAS verification emojis in the verification modal with a right-wrapping grid of cells, each displaying a large emoji glyph above its description.
Why was this PR needed?
Users reported the emojis were too small to compare against another device. The root cause was that all seven emojis were concatenated into a single ModalBody label at body-text size (11.5pt), with no per-emoji layout. Now that Makepad supports well-aligned right-wrap flow layouts, each emoji can be rendered as an independent cell at a larger size. See #846.
What are the relevant issue numbers?
Closes #846
Screenshots
Does this PR meet the acceptance criteria?
Project builds and tested locally, (screenshots above). Used Claude to review code, manually reviewed each line.