-
Notifications
You must be signed in to change notification settings - Fork 337
Fixes #1808: Added UI Feedback on Poll Delay #1830
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
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR @MritunjayTiwari14,
Let's fix up the commit message by following the commit style guidelines.
Something like:
poll: Display UI feedback on poll delay.
<description>
Fixes: #1808
Might be a better way to write this.
Feel free to add details and helpful info in the commit description.
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.
Starting a thread in the #mobile-design channel might be useful to gain initial feedback on this.
b87d1bb
to
3ddc690
Compare
Thank you very much for Reviewing! @apoorvapendse |
Okay, I will look forward into this Channel for further feedback. |
Cool, let's line wrap the description to <= 70 characters per line. |
If we show a spinner, I'd expect it to appear on the poll, not blocking the user from reading their other messages. |
I will make sure to use line wrap in the Description from Now, thank you. |
Okay, I’ll work on implementing a spinner similar to the Zulip web poll system. |
Improve Poll Vote Handling: Optimistic Updates with Store SynchronizationVideo Testing of Solutionrecord.mp4BackgroundWhile working on poll vote handling, we wanted the UI to feel responsive but also stay consistent with server-confirmed data. The requirement was:
Difficulties Faced
ResolutionAfter carefully studying the structure of the Poll model, the Model layer, and the API contract, we realized that a clean and reliable solution could be achieved by adopting optimistic updates with store synchronization:
Outcome
|
Extremely sorry for not removing the import that i used in my previous commit but then refactored my code so that it was not required which caused CI error. I have fixed that in the latest commit. |
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.
We don't want to keep merge commits in the PR branch.
Can you rebase this branch with upstream/main?
Sure! |
880bc90
to
536cbb7
Compare
@apoorvapendse Extremely Sorry for the inconvenience caused, when i tried to clean my commit history to start a fresh rebase. Git automatically closed this issue because it didn't found any commit from my origin/main. I have reopen the PR after rebasing upstream/main. |
5ad755c
to
af40234
Compare
af40234
to
3730c90
Compare
@gnprice It will be really helpful if I could get your feedback on the Updated Description of the Pull Request. |
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.
Thanks @MritunjayTiwari14, and thanks @apoorvapendse for the previous reviews! A couple of comments below.
Linking also the #mobile-design thread here: #mobile-design > loading indicator on voting in poll
color: Colors.transparent, | ||
child: InkWell( | ||
onTap: () => _toggleVote(option), | ||
child: Ink( |
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.
What's the effect of this Ink widget? Does the UI behave any differently if you take it out?
This catches my eye because we don't currently use an Ink widget anywhere else in the app (while we use Material and InkWell all the time).
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.
"The Ink widget can be used as a replacement for Image, Container, or DecoratedBox to ensure that the image or decoration also paints in the Material itself, below the ink."
Here is what i read on flutter on Here before implementing this. I basically implemented this as extras so that it does not cause any technical dept if we want to customize the Poll Widget Button Decoration even more in the future.
However as per the Feedback/Zulip flutter coding, the Ink Splash Effect will still technically work without Ink Widget hence to match the codebase style i am removing it in the upcoming commit.
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.
I see, so the point is that if there's an image or decoration painted on the button, then an Ink widget ensures that the ink splash can paint on top of that image or decoration, rather than underneath it.
And yeah, there's no need to use Ink in advance if we don't need it. If we do in the future change this decoration so that we need it, we can always add it then. This line is right next to the decoration, after all, so it'll be easy to find.
But I'm not actually convinced we don't already need it. This decoration does include a background color, which seems like it might be enough to make Ink necessary. Have you tried both versions with and without, and checked to see that they behave the same?
I'd be particularly interested in seeing a screen recording of how it looks in light theme. In dark theme, that color is only 20% opaque:
colorPollVoteCountBackground: const HSLColor.fromAHSL(0.2, 0, 0, 0).toColor(),
so that even if we should be using Ink, we can mostly get away with not using it (it'd just look slightly wrong). But in light theme, it's 100% opaque:
colorPollVoteCountBackground: const HSLColor.fromAHSL(1, 0, 0, 1).toColor(),
so if we do need Ink, then the splash will be completely defeated if we don't use it.
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.
With Ink Widget
Ink.Widget.mp4
With Container Widget
Container.Widget.mp4
After Testing for both With and Without Ink Widget(i.e replacing the Ink Widget with Container Widget) in both Light and Dark Theme. We do really need Ink Widget here in place of Container for Ink Splash to be visible in both the themes.
In Light Mode, the background colors of the container becomes completely opaque which leads to the invisibility of the Ink Splashes.
This is because even though Ink Splashes are rendered by InkWell. The Splash are painted onto Material, so if there is any opaque widget between Material and InkWell or any child of InkWell having opaque decoration then the splashed will be not visible.
So to troubleshoot this issue, we should use Ink Widget which all us to make the splash visible over the opaque decoration.
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.
With Ink Widget
Ink.Widget.mp4
Yes, With Ink Widget
looks and works great! 👍
@gnprice I have added new commit getting the changes, by the way It's Absolutely my pleasure to be able to contribute to such awesome open source organization! |
Great work @MritunjayTiwari14 |
Thanks @MritunjayTiwari14 for the revision. See my reply above at #1830 (comment) — I'm not sure if this version works correctly, and would like to see screen recordings confirming that before we merge it. Please also take a look at the section in our README about how to structure your PR into Git commits:
If you have questions about how to do that, |
Absolutely @gnprice, the recording have been attached for your reference😊. Thank you for pointing out the opacity change in the background color in the light theme. |
In this commit, we added Material, InkWell and Ink Widget to account for the Ink Splash effect for UI feedback. Fixes: zulip#1808
Added Accidentally Deleted Comments by Android Studio IDE, replaced Ink Widget with Container Widget to match Zulip Codebase Style. Fixes: zulip#1808
Introduces Ink Widget Inplace of Container Widget to make Ink Splash visible over Container Widget Decoration. Fixes: zulip#1808
3a47c2e
to
f5c17a6
Compare
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.
Thanks @MritunjayTiwari14 for the revision! The video recordings are helpful.
One way to improve them, for future recordings you make, would be to cut them down to just the relevant few seconds. That way a reviewer doesn't have to sift through all the setup steps, and can more easily look closely at the relevant parts and re-watch those.
There's still some behavior to be fixed in this version; see comments below.
Please also revise the commit structure to meet our standards, as I've mentioned above:
Please also take a look at the section in our README about how to structure your PR into Git commits:
Your changes will need to be organized into clear and coherent commits, following Zulip's commit style guide. (Use Greg's "secret" to using
git log -p
and/or a graphical Git client to see examples of mergeable commits.)If you have questions about how to do that,
#git help
on chat.zulip.org is the best venue to ask.
child: Material( | ||
color: Colors.transparent, |
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.
This is forbidden by the docs for this property:
/// The color to paint the material.
///
/// Must be opaque. To create a transparent piece of material, use
/// [MaterialType.transparency].
You can find that doc on the web here:
https://main-api.flutter.dev/flutter/material/Material/color.html
but when developing the code, there's an easier way still: use "jump to definition" in your IDE on the parameter name "color:" here, and then use it again to get to the declaration final Color? color;
in the upstream source code. The documentation appears there, at the definition.
(For me in Android Studio "jump to definition" is bound to Ctrl+click, and also to Ctrl+B; you can use whichever is more convenient. It might be different bindings for you.)
child: Material( | ||
color: Colors.transparent, | ||
child: InkWell( | ||
onTap: () => _toggleVote(option), | ||
child: Ink( | ||
// Inner padding preserves whitespace even when the text's | ||
// width approaches the button's min-width (e.g. because | ||
// there are more than three digits). | ||
padding: const EdgeInsets.symmetric(horizontal: 4), | ||
decoration: BoxDecoration( | ||
color: theme.colorPollVoteCountBackground, | ||
border: Border.all(color: theme.colorPollVoteCountBorder), | ||
borderRadius: BorderRadius.circular(3)), |
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.
This feels more convoluted than it needs to be:
- We make a layer of "material".
- Then there's an InkWell, which is going to sometimes draw on the material.
- Then there's an Ink, and its job is to tell the Material to draw some things there.
They're all right next to each other, and meant to cover the exact same area. Instead of having the Ink tell the Material what to draw, why not tell the Material to draw the same things from the beginning?
(The Ink would be useful, on the other hand, if it were covering only part of the Material. That seems to be the situation it's documented as being intended for.)
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.
There's also a user-visible glitch that happens basically because of this complicated structure: when pressing on the button, the ink splash goes beyond the boundaries of the button to spill out to the corners (beyond the rounded corners of the button itself).
This is visible in the video above if you look closely. It becomes easier to see if you change BorderRadius.circular(3)
to use a much bigger number, so that there's more rounding. I recommend doing that during development (but then change it back to 3 for the version you send in your PR).
Take a look at the different fields that the Material
widget has, to see what controls it offers you. Using those, you can get the ink splash to behave correctly with the rounded corners.
Pull Request: [Ink Splash Poll Vote Indicator Implementation](polls: Add UI feedback on vote button while request pending #1808)
Hello Moderators of Zulip Flutter,
This is Mritunjay Tiwari.
This is my first ever contribution to Zulip.
📌 My Solution
Implementation Video and Images
test_video.mp4
After looking at the Suggestion for the issue made by the moderators, implementation of a Ink Splash upon Tap on Poll Vote is successfully done.
So, the implementation of the Ink Splash Widget is done by replacing the container of the Poll Vote button with a Ink Widget and introducing InkWell and Material Widgets as its ancestors. Making the Material as its Ancestors allows the Splash on of the Ink on the Button which eliminate the risk of leaking of the Splash outside the Poll Vote Button.
Thank you for reviewing my contribution!
💻 Code Snippet
(Added Comments here only to explain the changes, Review the diff for the changes without the addition of the comments)