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

[fix] Fixed transparent element of notification widget that covered page #130 #131

Merged
merged 1 commit into from Sep 30, 2020

Conversation

niteshsinha17
Copy link
Member

The issue was related to div having class ".ow-notifications". It was taking css property bottom=-1 and it was positioned absolutely that's why it was taking complete height. So it made bottom = auto.

Fixes #130

@atb00ker atb00ker added this to To do (general) in OpenWISP Contributor's Board via automation Sep 20, 2020
@atb00ker atb00ker moved this from To do (general) to Needs review in OpenWISP Contributor's Board Sep 20, 2020
Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

Thanks for contributing @niteshsinha17.

This surely fixes the bug, but I don't think it is the appropriate solution. The problem occurs due to bottom: 1px in ow-notifications class. From what I can see, it is there to align the notification bell with the text, see below.

Screenshot from 2020-09-20 22-37-22

I think we can get the same effect with padding: 1px 20px 0px 1px and removing bottom: -1px in ow-notifications. With this, we will not need to add another property in the media query. The class will become something like this.

.ow-notifications {
    float: right;
    display: block;
    padding: 1px 20px 0px 20px;
    color: #fff;
    line-height: 40px;
    border: 0 none !important;
    position: relative;
    right: -5px;
    margin-left: 10px;
    cursor: pointer;
}

What do think about this?

OpenWISP Contributor's Board automation moved this from Needs review to In progress Sep 20, 2020
@niteshsinha17
Copy link
Member Author

Thanks for reviewing @pandafy.

I think whole setup is made to align user, nav-icon and bell-icon when width>1042px and to align nav-icon and bell-icon when width < 1024.
Screenshot (109)

Screenshot (110)

Your solution also solves the issue but it creates misalignment between bell-icon and nav-icon when width<1024 which is not in my solution.

Screenshot (111)

I have two solutions which don't need any addition of extra properties.

  1. remove bottom:-1px from ow-notifications , remove top:-1px from bell-icon. It will produce the same effect which is in your solution. To solve the misalignment of bell and nav icons, we can remove top:-1px from nav-icon.

  2. In your solution we can do one thing to solve the alignment issue. Make top:41px in ow-notifications in the media of max width:1024. So the classes will look like this.

.ow-notifications {
    float: right;
    display: block;
    padding: 1px 20px 0px 20px;
    color: #fff;
    line-height: 40px;
    border: 0 none !important;
    position: relative;
    right: -5px;
    margin-left: 10px;
    cursor: pointer;
}

@media (max-width: 1024px)
.ow-notifications {
    display: block !important;
    line-height: 40px !important;
    position: absolute;
    top: 41px;
    right: 113px;
}

What do you think about this?

Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

Thanks for your keen observation @niteshsinha17

  1. remove bottom:-1px from ow-notifications , remove top:-1px from bell-icon. It will produce the same effect which is in your solution. To solve the misalignment of bell and nav icons, we can remove top:-1px from nav-icon.

I will not prefer modifying CSS for nav-icon since it is not added in openwisp-notifications, it is a feature of openwisp-utils and it will be better to add any such modifications over there only. Honestly, I don't like this solution.

2. In your solution we can do one thing to solve the alignment issue. Make top:41px in ow-notifications in the media of max width:1024. So the classes will look like this.

This solution sounds more appropriate to me, but it should be top: 42px instead of top: 43px. You can go ahead with this one.

@niteshsinha17
Copy link
Member Author

Thanks @pandafy

I also like the 2nd solution more.

Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @niteshsinha17 !

Merging deferred to @nemesisdesign 😄

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Thanks a lot @niteshsinha17 and @pandafy 👏 👍

@nemesifier nemesifier merged commit c110d41 into openwisp:master Sep 30, 2020
OpenWISP Contributor's Board automation moved this from In progress to Done Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Transparent element of notification widget covers part of the page on < 1024px
3 participants