zulip
zulip copied to clipboard
zulip
todo_widget: Disable /todo add task button if duplicate task name or no task name.
This PR fixes https://github.com/zulip/zulip/issues/20211.
Testing plan: Visual testing by trying to interact with add task button with no task name present and checking that it does get disabled with tooltip. Visual testing with duplicate task name and interacting with button to see that it is disabled with tooltip.
GIFs or screenshots: GIF of no task name present. https://user-images.githubusercontent.com/36671046/143097792-34c83b54-6f1b-4858-a63b-16a05644a3cd.mp4
GIF of duplicate task name. https://user-images.githubusercontent.com/36671046/143097808-ad01c77a-5017-4ed2-82e9-5016eb821d8c.mp4
Hi @Sarah-Addo !
The experience is not quite right for blank tasks. The button should be disabled all the time in that case. In the current implementation, it only becomes disabled when you hover over it.
Actually, hm, after I wrote this comment, I realized I'm not so sure!
I think the button greying out when you hover over it looks odd, but it may also be confusing to have it greyed out to begin with.
Yes
-Tim Abbott (mobile)
On Wed, Nov 17, 2021, 11:30 Alya Abbott @.***> wrote:
@timabbott https://github.com/timabbott for the tooltips, should we be using tippy here?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zulip/zulip/pull/20261#issuecomment-971905414, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAU6NWVYGRU7IJPR5QRHOITUMP7HBANCNFSM5ICT3U3Q .
Actually, hm, after I wrote this comment, I realized I'm not so sure!
I think the button greying out when you hover over it looks odd, but it may also be confusing to have it greyed out to begin with.
I also had the same thought about it being confusing to have it grayed out to begin with. The button could be disabled upon creation of the todo list with a tooltip that instantly appears? I am currently looking into tippy for the tooltips but if you feel like there are any examples that would be helpful feel free to link them.
We use tippy for a lot of tooltips throughout the app, so you should definitely be able to find some examples.
I also had the same thought about it being confusing to have it grayed out to begin with. The button could be disabled upon creation of the todo list with a tooltip that instantly appears?
Hm, that seems like it might be unnecessarily invasive? Maybe the way you implemented is actually the best option. :)
@alya I've changed the tooltips to tippy tooltips and have updated the GIFs linked!
Cool, thanks!
Playing with it a bit more, let's try making the disabled button look like the disabled buttons in our settings menus. For example, this button in the Organization profile tab is disabled if you're not an org owner:
If you play with it, you'll see that the tooltip is different from an active button, and the text color changes when you hover over it.
I've styled the disabled button accordingly but I was wondering if you would like the disabled cursor symbol to appear since it seems to overlap with the tippy tooltip depending on how the user hovers over it which you can see here. Possible solution, if you think its an issue, is to change the cursor to the default pointer like so. Thoughts?
I think the overlap isn't too bad, and it's better to be consistent.
Let me know when it's ready for another review. :)
Cool, thanks! The button looks good, but I noticed that the tooltip sometimes gets cut off. Is it somehow behind an element that it should be in front of?
Yep it looks like when the list was empty the absolute positioning of the element was causing the cut off. I changed the positioning to relative and updated the css to ensure that the button still has the disabled effects when you hover over the wrapper which you can see here
Thanks for working on this @Sarah-Addo! I posted a few comments; it'd also be great if you can clean up the commit history (possibly by squashing the commits; I haven't tried to read them individually).
Check out our GitHub guide and commit guidelines for more details.
I've addressed the comments left and this is ready for another review :)
Heads up @Sarah-Addo, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.
Heads up @Sarah-Addo, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.