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

Fixed notifications position #10525

Merged
merged 1 commit into from
Aug 8, 2018
Merged

Fixed notifications position #10525

merged 1 commit into from
Aug 8, 2018

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Aug 3, 2018

Signed-off-by: John Molakvoæ (skjnldsv) skjnldsv@protonmail.com

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv added bug design Design, UI, UX, etc. 3. to review Waiting for reviews regression 14-beta-feedback labels Aug 3, 2018
@skjnldsv skjnldsv self-assigned this Aug 3, 2018
@skjnldsv skjnldsv added this to the Nextcloud 14 milestone Aug 3, 2018
@rullzer rullzer mentioned this pull request Aug 6, 2018
58 tasks
@ChristophWurst
Copy link
Member

How do I test this?

@skjnldsv
Copy link
Member Author

skjnldsv commented Aug 6, 2018

@ChristophWurst find a page who scrolls, create a temp notification, scroll and see the notification fly up as you scroll
With this patch, it stays at the top :)

@ChristophWurst
Copy link
Member

Thanks! Can verify the patch working. Isn't this fixing the visual bug I was asking you about a few days ago and you said there was no bug? 😛

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Perfect 🙌

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 7, 2018
@skjnldsv
Copy link
Member Author

skjnldsv commented Aug 7, 2018

and you said there was no bug? 😛

telechargement

@danopz
Copy link

danopz commented Aug 7, 2018

Does not work for me with multi line notifications or multiple ones.

Just copied the style using dev-tools and added some notifications with OC.Notification.show('foobar').
The container is in the top most layer and makes it impossible to use the navigation and some elements.

@skjnldsv
Copy link
Member Author

skjnldsv commented Aug 7, 2018

@danopz the notification always have been on top of the header. This fixes the scrolling issue on the notification.
Aside from the (very) unpractical way the notifications are displayed, is there anything wrong? :)

@rullzer rullzer merged commit 75de67a into master Aug 8, 2018
@rullzer rullzer deleted the notifications-fix branch August 8, 2018 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug design Design, UI, UX, etc. regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants