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

Rendering web notifications to the UI and Notification Inbox #385

Merged
merged 4 commits into from Aug 7, 2019

Conversation

@aquibbaig
Copy link
Collaborator

aquibbaig commented Jul 8, 2019

This is subject to further discussions regarding the design in the meeting today. I am adding that in the agenda.
TODO: Add Mark as seen and/or Remove Notification buttons

@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 8, 2019

Screenshot 2019-07-08 at 3 11 47 PM

Currently, what it shows:

  • The username and the notification settings handler on top
  • Notification list: Notification Message and Notification date(createdAt)
@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from 0aa945d to 1611996 Jul 9, 2019
@aquibbaig aquibbaig added the gsoc label Jul 9, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 9, 2019

Progress: Added Mark as seen and Remove Button
Screenshot 2019-07-09 at 10 26 27 PM

@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 9, 2019

Just had a check on a mobile viewport too

Screenshot 2019-07-09 at 10 29 01 PM

@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch 4 times, most recently from 6e9e494 to c663e51 Jul 9, 2019
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jul 10, 2019

Thanks for the UI proposal. I think we can simplify it a bit. How about this:

  • Only show unseen notifications in the box.
  • Remove the "Delete" functionality from the popup.
  • Only keep the "seen" option.
  • Remove the "eye" icon on the left, as now only unseen notifications are presented.
  • Add time to the date.
  • Make date/time less prominent, and use "relative dates" (we already include a library for that)

Furthermore, make sure that the notifications are "clickable", bringing the user to the "notification inbox". Do you have a screenshot for this page already?

@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from a71ccba to bfb0ce6 Jul 10, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 10, 2019

@imphil I had not given much attention to the "Notification inbox" yet and that's the reason why you can see that it is not implemented yet. So, we would want a Notification inbox page where all the Notifications of the User reside along with these 2 handlers to mark them seen and remove them?

@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 10, 2019

Also, the CSS in the first commit needs to be fixed. I will handle that!

@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jul 10, 2019

Sounds about right, but always try to think about simplifying the user experience. For example, you can avoid a "mark as seen" button if you just mark a notification as seen by clicking on it -- that's pretty standard in any email program. And the delete button is rather rarely used, so make it sufficiently small.

@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch 4 times, most recently from 4834561 to 9f74a7d Jul 10, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 10, 2019

Thanks, @imphil for your suggestions on this pull request. After the meeting, I switched to this UI, let me know if there is anything bugging you in the design.
notification-list

@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from 9f74a7d to ce314f4 Jul 10, 2019
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jul 10, 2019

The UI looks good now, with a minor nit on the text at the bottom: "See all Notifications" -> "show all notifications"

@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jul 10, 2019

I'll have a look at the code early tomorrow (UK time)

@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from ce314f4 to 8db5952 Jul 11, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 11, 2019

@imphil Regarding routing inside the controllers, there are two pages to design now

  • Notification Inbox
  • Notification Settings

These two things are user preferences in my opinion and should be routed to /user/notification/settings and /user/notification/inbox rather than normal ones. So do you prefer moving the routing logic to UserController instead?

If not, the user is routed onto /notification/inbox and /notification/settings which can be handled by our NotificationController.

@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from 8db5952 to f0bf1f0 Jul 11, 2019
@aquibbaig aquibbaig changed the title Rendering web notifications to the UI Rendering web notifications to the UI and Notification Inbox Jul 11, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 11, 2019

Adding some CSS fixes right now to differentiate seen notifications from unseen!

@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 11, 2019

Can't seem to do that at the moment cause I can't open any page rn due to
algoliaException\Hosts Unreachable, which is most likely a result of either bad internet or some issues with my algolia app that I had created. I'll look into it first thing tomorrow morning

@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jul 12, 2019

On routing: We cannot use /notification, as this is outside of our available namespace for site pages. /user/notifications or /user/inbox is fine for the inbox, and settings should be /user/settings/notifications, in line with all the other settings pages we have today.

@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from f0bf1f0 to 2ee5c37 Jul 12, 2019
Copy link
Contributor

imphil left a comment

  • Keep all lines at 80 characters typically, and 120 characters max. Easy check: If you need to horizontally scroll when looking at the diff in GitHub, your lines are too long.
  • Indent HTML/CSS/JS with two spaces. notification_list.html.twig and notification_inbox.html.twig are indented with a mix of four or more spaces.
site/app/Resources/views/base.html.twig Outdated Show resolved Hide resolved
site/app/Resources/views/base.html.twig Outdated Show resolved Hide resolved
site/app/Resources/views/base.html.twig Outdated Show resolved Hide resolved
site/assets/scss/app.scss Outdated Show resolved Hide resolved
site/assets/scss/notification_inbox.scss Outdated Show resolved Hide resolved
site/src/Controller/UserController.php Outdated Show resolved Hide resolved
site/src/Controller/UserController.php Show resolved Hide resolved
@aquibbaig aquibbaig requested a review from imphil Jul 31, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch 3 times, most recently from dc6debe to 0699246 Jul 31, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from 0699246 to 4014f06 Aug 1, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 1, 2019

@imphil just edited the commits w.r.t. yesterday's discussion. Let me know what you think

@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch 2 times, most recently from b87fcee to d6b2d0d Aug 1, 2019
{% for notification in notificationList|filter(notification => notification.seen == false) -%}
{#Render a unseen notification #}
{% if (notification.seen == false) %}
<ul class="unseen-notifications" id="notification-{{ notification.notification.id }}">

This comment has been minimized.

Copy link
@imphil

imphil Aug 1, 2019

Contributor

Please don't misuse ul/li as generic block element here and in the base.html.twig. Use lists when there are actually things to list (like a list of notifications), but use <div> if you only have blocks next to each other.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Aug 1, 2019

Author Collaborator

@imphil I had some checks on how others do it and this seems to be a general case(for bootstrap > 3.0). We just have a twig renderer that contains <li> items. <li> can have a nested <ul> and then <li> inside as per documentation. Please have a small look at this Bootstrap snippet. So, are we misusing it still?

This comment has been minimized.

Copy link
@imphil

imphil Aug 1, 2019

Contributor

ul/li is semantic markup for a list. A list of notifications is a list. A list of elements in a navigation bar is a list. Use ul/li there. But the code you have here is not a list: it's just different paragraphs: a subject, a message, a date, a divider. That's not a list, and therefore it should not use ul/li.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Aug 1, 2019

Author Collaborator

@imphil Sounds logical indeed, I'm using <span>, <p> and <div>s now

// Remove the Notification from the database
// TODO: Let foreign constraints take care of removing notifications, see #400
// $notificationManager->deleteNotification($notificationManager->getNotification($notification), true);

This comment has been minimized.

Copy link
@imphil

imphil Aug 1, 2019

Contributor

Wait: you commented this out, so it works without this line?

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Aug 1, 2019

Author Collaborator

Yup, it does. removeNotification() removes link between notifiableEntity and notifiableNotification, so if that is removed the user can't see it anymore i.e. it is not rendered as a part of notificationList which only renders notifications linked to the notifiableEntity which is now broken

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Aug 1, 2019

Author Collaborator

@imphil The deleteNotification() method is for removing the notification from the database itself

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Aug 1, 2019

Author Collaborator

It is a workaround probably

This comment has been minimized.

Copy link
@imphil

imphil Aug 1, 2019

Contributor

So the notification is still half-present in the database? That's not what we want, the notification should be fully gone. Sothe deleteNotification() part is necessary.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Aug 1, 2019

Author Collaborator

@imphil Exactly!

@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch 6 times, most recently from 0b5a879 to 1e16b11 Aug 1, 2019
@aquibbaig aquibbaig requested a review from imphil Aug 2, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from 3233f01 to 199ee3f Aug 4, 2019
@imphil
imphil approved these changes Aug 7, 2019
@imphil imphil merged commit b5fb652 into librecores:master Aug 7, 2019
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Aug 7, 2019

Thanks @aquibbaig, I've now merged your PR!

@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 20, 2019

Final Preview
notification_inbox

@aquibbaig aquibbaig deleted the aquibbaig:notifications branch Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.