Conversation
szaimen
requested review from
a team,
PVince81,
artonge and
juliusknorr
and removed request for
a team
|
/backport to stable22 |
|
/backport to stable21 |
|
/backport to stable20 |
|
/compile amend / |
Signed-off-by: szaimen <szaimen@e.mail.de> Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
nextcloud-command
force-pushed
the
fix/28584/fix-search-popups
branch
from
4b37f4f to
572afa8
Compare
PVince81
left a comment
PVince81
left a comment
There was a problem hiding this comment.
👍
assuming you also retested on mobile to make sure the other fix you did for it still works
PVince81
added
4. to release
|
Yes, it was tested successrully on mobile, too. But after thinking about it a bit more does the carret need a bit more love... |
szaimen
added
2. developing
| position: fixed; | ||
| z-index: 2000; | ||
| top: 50px; | ||
| right: -150px; | ||
| right: 0px; |
There was a problem hiding this comment.
We use the same design for non-vue header menu, no? Can you double check those changes are also applied properly?
Iirc the main issue with a fixed position is that it will not be relative to the button as per the original design requirements.
The change applies on the mobile view:
Lines 141 to 148 in cfb94c7
There was a problem hiding this comment.
This css code is unfortunately not applied on this element...
There was a problem hiding this comment.
Yes, this is what is missing.
When the screen is narrower than 480, then it should be fixed :)
There was a problem hiding this comment.
You can just copy/paste the style from the mobile.scss and put it in the HeaderMenu component 😉
There was a problem hiding this comment.
Thats what I am talking about at my last comment down below:
The best solution would be if the carrot wasn't part of the element but instead created by the element that triggers the popup so that the carrot can be positioned absolute to the trigger element and the popup can be positioned fixed on the site.
This is excactly how the legacy code does it. See:
The solution that the legacy mobile.scss code uses is not possible here because the carrot is part of the element.
Please also check the git blame/review suggestions, as the author of this file, I should have been requested for reviews to avoid regressions 😉 |
Will do that in the future. I think I didn't ping you in the other one because you seemed to be very busy... |
|
@skjnldsv I currently see no way to fix this for everyone (for mobile and for people that disabled the notifications app). The best solution would be if the carrot wasn't part of the element but instead created by the element that triggers the popup so that the carrot can be positioned absolute to the trigger element and the popup can be positioned fixed on the site. But this is unfortunately a bit over my level of skill. Maybe @Pytal or @artonge can help me here? |
|
Another attempt is in #28660 |
4 participants
Fix #28584
Signed-off-by: szaimen szaimen@e.mail.de
(I broke this in #27844)