Skip to content

fix the search popup for everyone - #28585

Closed
szaimen wants to merge 1 commit into
masterfrom
fix/28584/fix-search-popups
Closed

fix the search popup for everyone#28585
szaimen wants to merge 1 commit into
masterfrom
fix/28584/fix-search-popups

Conversation

@szaimen

@szaimenszaimen commented Aug 24, 2021

Copy link
Copy Markdown
Contributor

Fix#28584
Signed-off-by: szaimen szaimen@e.mail.de
(I broke this in #27844)

@szaimenszaimen added bug design Design, UI, UX, etc. labels Aug 24, 2021
@szaimenszaimen added this to the Nextcloud 23 milestone Aug 24, 2021
@szaimen
szaimen requested review from a team, PVince81, artonge and juliusknorr and removed request for a teamAugust 24, 2021 13:55
@szaimen

Copy link
Copy Markdown
ContributorAuthor

/backport to stable22

@szaimen

Copy link
Copy Markdown
ContributorAuthor

/backport to stable21

@szaimen

Copy link
Copy Markdown
ContributorAuthor

/backport to stable20

@szaimen

Copy link
Copy Markdown
ContributorAuthor

/compile amend /

Signed-off-by: szaimen <szaimen@e.mail.de>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@nextcloud-command
nextcloud-commandforce-pushed the fix/28584/fix-search-popups branch from 4b37f4f to 572afa8CompareAugust 24, 2021 14:22
@szaimenszaimen added the 3. to review Waiting for reviews label Aug 24, 2021

@PVince81PVince81 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

assuming you also retested on mobile to make sure the other fix you did for it still works

@PVince81PVince81 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 25, 2021
@szaimen

Copy link
Copy Markdown
ContributorAuthor

Yes, it was tested successrully on mobile, too. But after thinking about it a bit more does the carret need a bit more love...

@szaimenszaimen added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Aug 25, 2021
Comment on lines +187 to +190
position: fixed;
z-index: 2000;
top: 50px;
right: -150px;
right: 0px;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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:

@mediaonlyscreenand (max-width: 480px) {
#header.header-right > div > .menu {
max-width: calc(100vw-10px);
position: fixed;
&::after {
display: none!important;
}
}

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This css code is unfortunately not applied on this element...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, this is what is missing.
When the screen is narrower than 480, then it should be fixed :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can just copy/paste the style from the mobile.scss and put it in the HeaderMenu component 😉

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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:
image
image

The solution that the legacy mobile.scss code uses is not possible here because the carrot is part of the element.

@skjnldsv

Copy link
Copy Markdown
Member

szaimen requested review from nextcloud/server-frontend, PVince81, juliushaertl and artonge and removed request for nextcloud/server-frontendil y a 19 heures

Please also check the git blame/review suggestions, as the author of this file, I should have been requested for reviews to avoid regressions 😉
image

@szaimen

Copy link
Copy Markdown
ContributorAuthor

Please also check the git blame/review suggestions, as the author of this file, I should have been requested for reviews to avoid regressions 😉
image

Will do that in the future. I think I didn't ping you in the other one because you seemed to be very busy...

@szaimen

Copy link
Copy Markdown
ContributorAuthor

@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?

@szaimen

Copy link
Copy Markdown
ContributorAuthor

Another attempt is in #28660

@szaimen
szaimen deleted the fix/28584/fix-search-popups branch December 1, 2023 08:31
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2. developingWork in progressbugdesignDesign, UI, UX, etc.help wantedregression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NC 22.1.0 - Search modal

4 participants

@szaimen@skjnldsv@PVince81@Pytal