[ux] Restyle notification widget #182 #184

niteshsinha17 · 2021-06-10T07:17:51Z

Changes

Expected design: https://miro.com/app/board/o9J_lMAQQZk=/

Restyled notification widget.
Changed unsubscribe button.
Updated CI run commands because tests were failing.
Updated readme.
Manually tested current changes.

Tasks

Create a design for the widget.
Implement the created design.

Checklist

I have read the contributing guidelines.
I have manually tested the proposed changes.
I have written new test cases to avoid regressions. (if necessary)
I have updated the documentation. (e.g. README.rst)

close #182

niteshsinha17 · 2021-06-10T10:00:09Z

@nemesisdesign and @atb00ker I have made some changes in ci because it was not installing the correct version of utils and users so tests were falling. But that was not an issue.

Problem is that menu element is getting registered twice at the same position from users during the test. Please check test's output. Do you have any idea why it's happening? I am facing the same issue with controller.

nemesifier

@niteshsinha17 that's a special test which messes with the installed apps, please read the code of the test to understand what it does so you should figure out a solution.

atb00ker

I have not tested, but some comments:

[chores] Minnor changes -> [chores] Minor changes
Do we want to merge this to master or create a branch? @nemesisdesign

openwisp_notifications/static/openwisp-notifications/images/icons/icon-bell.svg

openwisp_notifications/static/openwisp-notifications/js/notifications.js

atb00ker · 2021-06-19T13:05:50Z

Hey @niteshsinha17 commit messages are not fixed as per the last review, do you need help with it?

niteshsinha17 · 2021-06-19T14:12:11Z

Hey @niteshsinha17 commit messages are not fixed as per the last review, do you need help with it?

Thanks, I have changed them. 😄 I thought you were saying about the typo that I should not repeat.

atb00ker

Too many unrelated changes, I think some of them could have been seperately taken care. 😄

openwisp_notifications/static/openwisp-notifications/js/object-notifications.js

atb00ker · 2021-06-23T14:50:45Z

requirements.txt

-openwisp-utils[rest]
+openwisp-users @ https://github.com/openwisp/openwisp-users/tarball/issues/253-register-menu-groups
+# TODO set the next point release of openwisp-utils when ready
+openwisp-utils[rest] @ https://github.com/niteshsinha17/openwisp-utils/tarball/gsoc/new-navigation-menu-design


Before merge, please make it point to openwisp/openwisp-utils from niteshsinha17/openwisp-utils.

atb00ker

I am confused about what should I test here:

Design:

What I see:

There is a significant UI & it's feel difference, honestly, I don't mind either but am I even checking the correct thing?

The buttons in the currently visible section is too large, can we make them smaller please?
Can we hide these buttons when there are no new notifications in the new design?

niteshsinha17 · 2021-06-24T02:32:49Z

@atb00ker Please don't judge it by comparing it with design because in miro we have some limitations. We can improve it during the implemetation.

The buttons in the currently visible section is too large, can we make them smaller please?

👍

Can we hide these buttons when there are no new notifications in the new design?

It was already implemented in that way. Should we change it?

atb00ker · 2021-06-24T20:08:42Z

It was already implemented in that way. Should we change it?

I think it wasn't implemented before, but I think it makes sense in the new UI! 😄
We can create a seperate issue for it and tackle on low priority later on, sounds good? 😄

niteshsinha17 · 2021-07-01T11:56:26Z

It was already implemented in that way. Should we change it?

I think it wasn't implemented before, but I think it makes sense in the new UI!
We can create a seperate issue for it and tackle on low priority later on, sounds good?

Yes, It's a good idea. I will open an issue for it.

nemesifier

Please ensure z-index is correctly set, otherwise map tiles can cover the notification widget:

atb00ker · 2021-07-12T17:53:31Z

openwisp_notifications/templates/admin/base_site.html

    {% unread_notifications %}
  </div>
  {% csrf_token %}
  {% notification_toast %}
  {% notification_widget %}
-{% endblock %}
-
+{% endblock  %}


Change not required.
P.S If you haven't already, install linters and if you use vscode, add:

"files.insertFinalNewline": true, "files.trimTrailingWhitespace": true,

to ensure file ends with a new line and trailing whitespaces are removed!

Thank you 😄

atb00ker

The "mark all as read" and "Show unread only" options look like they are borrowed from the UI. Is there any reason why entire UI isn't like other buttons? (they are more round and turn a different shade of grey when in focus).
But it's going in the correct direction! 👍

nemesifier

Looks like it's on the right track. I've added some minor improvements in 68df533.
Will be doing more review and testing in the coming days.

nemesifier

Something odd I noticed when testing on openwisp-monitoring, I see 2 "send command" buttons in the device detail page, it doesn't happen on master, can you please double check?

nemesifier

Notification icon

Could you change the notification widget icon to use the same technique of masking so we can change its color on hover/focus please?

[deps] Updated utils and users

f41a722

niteshsinha17 self-assigned this Jun 10, 2021

nemesifier reviewed Jun 11, 2021

View reviewed changes

atb00ker reviewed Jun 17, 2021

View reviewed changes

openwisp_notifications/static/openwisp-notifications/images/icons/icon-bell.svg Outdated Show resolved Hide resolved

openwisp_notifications/static/openwisp-notifications/js/notifications.js Outdated Show resolved Hide resolved

niteshsinha17 added 7 commits June 19, 2021 19:37

[ci] Minor changes in installation process

ec852d8

[enhancement] Restyled notification widget

fc130eb

[ci] Reset ci to old state

7806449

[chores] Minor changes

53ed629

[deps] Minor changes

dad349e

[chores] Improvements

afe7341

[chores] Formatted svg

86529ce

niteshsinha17 force-pushed the issues/182-restyle-notification-widget branch from f430a1f to 86529ce Compare June 19, 2021 14:08

atb00ker reviewed Jun 23, 2021

View reviewed changes

nemesifier requested changes Jul 3, 2021

View reviewed changes

[chores] Improvements after new menu design

38da181

atb00ker reviewed Jul 12, 2021

View reviewed changes

[chores] Minor improvements to notification widget CSS

68df533

nemesifier reviewed Jul 17, 2021

View reviewed changes

nemesifier requested changes Jul 20, 2021

View reviewed changes

[change] Changed button styles

f97ce9a

[chores] Minor improvements

e561747

nemesifier requested changes Jul 26, 2021

View reviewed changes

[ui] Added mask to notification icon

9129c2d

niteshsinha17 force-pushed the issues/182-restyle-notification-widget branch from 2fc752d to 9129c2d Compare July 27, 2021 18:18

nemesifier approved these changes Jul 29, 2021

View reviewed changes

nemesifier and others added 3 commits July 29, 2021 14:24

[chores] Use dropdown style defined in utils

7f15595

[chores] Minor adjustment

e6da17d

[deps] Point utils to master

587f960

niteshsinha17 force-pushed the issues/182-restyle-notification-widget branch from 9ec8359 to 587f960 Compare July 30, 2021 03:13

nemesifier merged commit b0b5522 into master Aug 1, 2021

nemesifier deleted the issues/182-restyle-notification-widget branch August 1, 2021 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ux] Restyle notification widget #182 #184

[ux] Restyle notification widget #182 #184

niteshsinha17 commented Jun 10, 2021 •

edited

Loading

niteshsinha17 commented Jun 10, 2021

nemesifier left a comment

atb00ker left a comment

atb00ker commented Jun 19, 2021

niteshsinha17 commented Jun 19, 2021

atb00ker left a comment

atb00ker Jun 23, 2021

atb00ker left a comment

niteshsinha17 commented Jun 24, 2021

atb00ker commented Jun 24, 2021

niteshsinha17 commented Jul 1, 2021

nemesifier left a comment

atb00ker Jul 12, 2021

niteshsinha17 Jul 22, 2021

atb00ker left a comment

nemesifier left a comment

nemesifier left a comment

nemesifier left a comment

[ux] Restyle notification widget #182 #184

[ux] Restyle notification widget #182 #184

Conversation

niteshsinha17 commented Jun 10, 2021 • edited Loading

Changes

Tasks

Checklist

niteshsinha17 commented Jun 10, 2021

nemesifier left a comment

Choose a reason for hiding this comment

atb00ker left a comment

Choose a reason for hiding this comment

atb00ker commented Jun 19, 2021

niteshsinha17 commented Jun 19, 2021

atb00ker left a comment

Choose a reason for hiding this comment

atb00ker Jun 23, 2021

Choose a reason for hiding this comment

atb00ker left a comment

Choose a reason for hiding this comment

niteshsinha17 commented Jun 24, 2021

atb00ker commented Jun 24, 2021

niteshsinha17 commented Jul 1, 2021

nemesifier left a comment

Choose a reason for hiding this comment

atb00ker Jul 12, 2021

Choose a reason for hiding this comment

niteshsinha17 Jul 22, 2021

Choose a reason for hiding this comment

atb00ker left a comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Notification icon

niteshsinha17 commented Jun 10, 2021 •

edited

Loading