Skip to content
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

Add mobile notifications and adapt its design #1890

Merged
merged 19 commits into from
Feb 2, 2022
Merged

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Jan 28, 2022

closes #1881
Did my best to stick to it the mock. there might be slight differences like shadows on the notification button (the mock has some, not mine), or on the notification panel (the mock doesn't have any, mine has some). But those are deliberate to keep consistency for the buttons, and to show that the notification panel is above the rest, otherwise it wasn't clear IMHO. Also I did not expand the notification panel all the way so that it's easier to click out. lmk your thoughts.

ping @serenaho if you want to play with it as well and tell me what I forgot/missed.

How to get a notification:

  • go to settings > subscriptions
  • change plan and chose a more expensive one
  • choose annual billing and crypto billing
  • go until the screen where you can chose the currency
  • from that point in time you should have a "outstanding invoice" notification, for 1 hour.

Submission checklist:

Layout

  • Change looks good in the desktop web ui
  • Change looks good in the mobile web ui

Theme

  • Components / elements inspected in light mode
  • Components / elements inspected in dark mode

@render
Copy link

render bot commented Jan 28, 2022

@render
Copy link

render bot commented Jan 28, 2022

@render
Copy link

render bot commented Jan 28, 2022

@github-actions github-actions bot added the Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes. label Jan 28, 2022
@Tbaut Tbaut changed the title Mnt/tbaut notif mobile 1881 Add mobile notifications and adapt its design Jan 28, 2022
@serenaho
Copy link
Collaborator

I reviewed the changes and things are looking good! Mostly small style fixes here and there (which unfortunately I cannot go back to review because of the 60 min hold) but these are the elements I noticed first:

  1. The padding of the Notification button is currently bigger compared to the other buttons and the red circle with the "1" doesn't go away when you click on it.

Screen Shot 2022-01-28 at 12 01 14 PM

  1. This modal has two "Go back" options (remove the underlined one)
    Screen Shot 2022-01-28 at 11 28 15 AM

  2. The hover for Notifications looks a bit slower compared to the other buttons

Notification.Hover.mov

@serenaho
Copy link
Collaborator

I also realized that there aren't currently any options to cancel a plan change once you start the crypto payment process.

I know the 60 minute timer eventually times out, but there's currently no way for the user to know that their plan status reverts to normal if they just wait (they might try to contact us).

Is it possible to cancel it once we come back to Settings like below?

image

If not, it may be worth it to put a message somewhere saying that the "Awaiting payment" status will go away.

*Side note, this might sound like an edge case but I imagine some users might be curious to see what the crypto payment process looks like and not realize it triggers something

@asnaith
Copy link
Member

asnaith commented Jan 28, 2022

I noticed something minor - when tapping to invoke the notification panel the icon turns blue but if you tap on the icon to dismiss the panel the icon remains blue until you tap the screen again elsewhere.

I think the colour should update on the tap that hides the panel rather than on the proceeding screen tap after that action.

screen-20220128-151516.mp4

I also noticed the persisting notification count number but I'm guessing that's because the actual notification persists too, is this behavior is expected until / if we have a clear-all button on the panel?

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jan 31, 2022

Thanks for the review, there's a lot to unpack here, I'll start with what I addressed:
@asnaith

when tapping to invoke the notification panel the icon turns blue but if you tap on the icon to dismiss the panel the icon remains blue until you tap the screen again elsewhere.

We were using a button so far, and this was somehow the standard behavior for buttons, linked to our "focus" styling.
I've set this notification to not be a button anymore, and made the style match with the others buttons, that's quite a lot of changes, for a small visual differences, so we should take a closer look.

@serenaho

The hover for Notifications looks a bit slower compared to the other buttons

I've a hard time reproducing, but I think I fixed it by removing the transition, can you check?

The padding of the Notification button is currently bigger compared to the other buttons

That should be fixed now

and the red circle with the "1" doesn't go away when you click on it.

This is not something I've set here, since I only took a particular care about mobile notifications. As Andrew mentioned, this is indeed something that we want, since this notification is very important. Unless you pay or let the 60min go by, you won't be able to dismiss it!

which unfortunately I cannot go back to review because of the 60 min hold

You can login with another user to have no notification again

This modal has two "Go back" options (remove the underlined one)

So the second "go back" is to close the "add card" section. We should find a better wording, but they are different.

I also realized that there aren't currently any options to cancel a plan change once you start the crypto payment process...

This is also a conversation that should be had outside of this PR, since it's not related 😄

Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks spotless 💯

I would suggest a few style changes regarding the notification dropdown itself.

Screenshot 2022-01-31 at 10 23 06 PM

Screenshot 2022-01-31 at 10 27 20 PM

  1. remove the left side padding, the notification text should align with This week heading.
  2. Allow enough space for the space for the "time" text
  3. Time text should also be right aligned.

One functionality update on my mind. I expect the notification dropdown to close when I click on a notification for example to go to the billing page.
We can add a closeOnClick: boolean key to the notification object.

Copy link
Contributor

@FSM1 FSM1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great stuff @Tbaut 🎉

@asnaith
Copy link
Member

asnaith commented Jan 31, 2022

We were using a button so far, and this was somehow the standard behavior for buttons, linked to our "focus" styling. I've set this notification to not be a button anymore, and made the style match with the others buttons, that's quite a lot of changes, for a small visual differences, so we should take a closer look.

I'm still seeing a difference in behavior between this notification icon and the buttons to the left of it. If you see the way to "start a team" button works - it stays blue whilst the dialog is in view but the blue background disappears when the modal is dismissed, this is what I had in mind for the notification icon and panel (sorry, I know this sounds nitpicky but thought consistency is best).

Screen.Recording.2022-01-31.at.10.13.07.AM.mov

@Tbaut
Copy link
Collaborator Author

Tbaut commented Feb 1, 2022

I'm still seeing a difference in behavior between this notification icon and the buttons

Sure, because now it's not a button any more. A button remains in focus when you click it, until you click away. This is exactly what you do in the video above. And this was also the original issue you mentioned, that made me change the behavior.

  • your video: click on the button, it turns blue because it's focused. Click on the modal, the button isn't focuses any more: it's not blue any more
  • previously: click on the notification button, it turns blue because it's focused. The notification drop down opens. click on the focused button again, it's still focused and blue, but since you clicked outside of the drop down, it closes.

All in all, I'm not sure what's better, the current (non button) or if I should revert. I'd be willing to forget about the confusion it creates, as small as it can be. In any case what we see is the normal behavior of a button.. and a confusion created by the fact that we have a focus looking the same as a hover. I believe we did this for the login originally, but that's another story.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Feb 1, 2022

agreed with your comments @tanmoyAtb I changed the padding to stick to the design (so I added a padding to the time header), aligned the time right, and leveraged the autoclose from the dropdown to make sure the panel closes when we click on something, even if that's a menu element, a click on anything closes the notification... It's a quick yet not dirty fix, hope it doesn't get in the way in the future. Now this PR has been tackling many somewhat unrelated to mobile changes, and we keep adding things, I'd like to merge and add anything unrelated to a dedicated issue :)

image

@Tbaut Tbaut requested a review from tanmoyAtb February 1, 2022 11:05
Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go to me

@asnaith
Copy link
Member

asnaith commented Feb 1, 2022

All in all, I'm not sure what's better, the current (non button) or if I should revert. I'd be willing to forget about the confusion it creates, as small as it can be. In any case what we see is the normal behavior of a button.. and a confusion created by the fact that we have a focus looking the same as a hover. I believe we did this for the login originally, but that's another story.

@Tbaut 👍 Ok, sorry to cause any confusion over something so minor. Happy to go with whatever you deem best either the current way or revert back to a button.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Feb 2, 2022

I achieved what is IMHO good UX, at the cost of a somewhat more complex dropdown component though. I'd need an ack from @tanmoyAtb or @FSM1 before merging just for sanity and making sure we agree on the approach.

active-notif.mp4

@tanmoyAtb
Copy link
Contributor

tanmoyAtb commented Feb 2, 2022

I achieved what is IMHO good UX, at the cost of a somewhat more complex dropdown component though. I'd need an ack from @tanmoyAtb or @FSM1 before merging just for sanity and making sure we agree on the approach.

active-notif.mp4

The hack looks sane to me. Keeping a state to know the dropdown is open or not looks like the straight forward approach. 👍

@Tbaut Tbaut merged commit 7938c17 into dev Feb 2, 2022
@Tbaut Tbaut deleted the mnt/tbaut-notif-mobile-1881 branch February 2, 2022 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mobile notification & search
6 participants