Skip to content

Fix position of search bar #28660

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.

Sign up for GitHub

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

Jump to bottom
Merged
CarlSchwan merged 1 commit into from
Aug 31, 2021
Merged

Fix position of search bar #28660

CarlSchwan merged 1 commit into from
Aug 31, 2021

Conversation

@CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Aug 31, 2021 •
edited by szaimen
Loading

Fix #28584
Close #28585

New:

image
image

Before:

image
image

Signed-off-by: Carl Schwan [email protected]

@CarlSchwan CarlSchwan requested review from a team, jancborchardt, julien-nc, juliusknorr and szaimen and removed request for a team August 31, 2021 11:34
@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label Aug 31, 2021
@szaimen
Copy link
Contributor

szaimen commented Aug 31, 2021

@CarlSchwan Can you please show screenshot of this when the notifications app is enabled?
I tried to fix this too in #28585 btw ;)

@szaimen szaimen mentioned this pull request Aug 31, 2021
Closed
@CarlSchwan CarlSchwan force-pushed the work/carl/fix-search-position branch 2 times, most recently from d444241 to 2f45ac9 Compare August 31, 2021 12:52
@CarlSchwan
Copy link
Member Author

CarlSchwan commented Aug 31, 2021

@CarlSchwan Can you please show screenshot of this when the notifications app is enabled?
I tried to fix this too in #28585 btw ;)

I didn't know you were already working on it :(

I figured a hacky way to make it work: using a CSS detection that the notifications element is not empty we can push the carret a bit more to the left.

Here is a screenshot with the notification stuff enabled

image

szaimen
szaimen approved these changes Aug 31, 2021
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Fine by me unless anyone finds a better way (less hacky) e.g. like suggested here:
#28585 (comment)

core/src/components/HeaderMenu.vue
.notifications:not(:empty) ~ #unified-search {
order: -1;
.header-menu__carret {
right: 175px;
Copy link
Contributor

@szaimen szaimen Aug 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Are you sure that this will work?
I think it will be overwritten by the value that you've defined below as css is cascading ;)
So this should be moved to the bottom of the file, imho.

Copy link
Member Author

@CarlSchwan CarlSchwan Aug 31, 2021

Choose a reason for hiding this comment

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

It works. Css is cascading but since the rule includes an id and is more specifics it is stronger :D

Copy link
Contributor

@szaimen szaimen Aug 31, 2021

Choose a reason for hiding this comment

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

Okay

Copy link
Member

@skjnldsv skjnldsv Jun 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

🧑‍⚖️ hum, this is not a great piece of maintainable code 🙈
#32702 (comment)

@szaimen szaimen requested review from Pytal, artonge and skjnldsv August 31, 2021 13:31
szaimen
szaimen reviewed Aug 31, 2021
View reviewed changes
@kesselb
Copy link
Contributor

kesselb commented Aug 31, 2021

Hey @488849, thank you for reviewing this pull request. Approving once is enough. We take care of the rest 🤞

@szaimen szaimen added this to the Nextcloud 23 milestone Aug 31, 2021
@szaimen
Copy link
Contributor

szaimen commented Aug 31, 2021

/backport to stable22

@szaimen
Copy link
Contributor

szaimen commented Aug 31, 2021

/backport to stable21

@szaimen
Copy link
Contributor

szaimen commented Aug 31, 2021

/backport to stable20

Pytal
Pytal approved these changes Aug 31, 2021
View reviewed changes
Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

Just slightly hacky 😂

@Pytal
Copy link
Member

Pytal commented Aug 31, 2021

/compile amend /

@nextcloud-command nextcloud-command force-pushed the work/carl/fix-search-position branch from 2f45ac9 to 565cf69 Compare August 31, 2021 14:29
@szaimen
Copy link
Contributor

szaimen commented Aug 31, 2021 •
edited
Loading

Why are there suddenly conflicts after compiling? The compiled files didn't change in master since creating this branch, afaik.

Could maybe this be the reason?

server/.github/workflows/command-compile.yml

Line 79 in 4411822

cache: npm

Cc @skjnldsv

@CarlSchwan CarlSchwan force-pushed the work/carl/fix-search-position branch from 565cf69 to c4c5bdd Compare August 31, 2021 15:05
@CarlSchwan
Copy link
Member Author

CarlSchwan commented Aug 31, 2021

/compile amend /

@CarlSchwan
Copy link
Member Author

CarlSchwan commented Aug 31, 2021

Why are there suddenly conflicts after compiling? The compiled files didn't change in master since creating this branch, afaik.

Could maybe this be the reason?

server/.github/workflows/command-compile.yml

Line 79 in 4411822

cache: npm

Cc @skjnldsv

I just rebased and let the compile bot generate the assets again, let see...

@CarlSchwan @nextcloud-command
Fix position of search bar
4027bba 
Signed-off-by: Carl Schwan <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
@nextcloud-command nextcloud-command force-pushed the work/carl/fix-search-position branch from c4c5bdd to 4027bba Compare August 31, 2021 15:12
@szaimen
Copy link
Contributor

szaimen commented Aug 31, 2021 •
edited
Loading

I just rebased and let the compile bot generate the assets again, let see...

Seems to work now but still strange...

@szaimen szaimen 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 31, 2021
@CarlSchwan
Copy link
Member Author

CarlSchwan commented Aug 31, 2021

ci seems stuck but also unrelated to this PR:

[1630425618.218][SEVERE]: bind() failed: Cannot assign requested address (99)
ChromeDriver was started successfully.

@CarlSchwan CarlSchwan merged commit 734a750 into master Aug 31, 2021
@CarlSchwan CarlSchwan deleted the work/carl/fix-search-position branch August 31, 2021 16:26
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Aug 31, 2021
Merged
@nextcloud nextcloud deleted a comment Aug 31, 2021
@nextcloud nextcloud deleted a comment Aug 31, 2021
@nextcloud nextcloud deleted a comment Aug 31, 2021
@nextcloud nextcloud deleted a comment Aug 31, 2021
@nextcloud nextcloud deleted a comment Aug 31, 2021
@szaimen
Copy link
Contributor

szaimen commented Aug 31, 2021

/backport to stable21

@szaimen
Copy link
Contributor

szaimen commented Aug 31, 2021

/backport to stable20

This was referenced Aug 31, 2021
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Jun 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@szaimen szaimen szaimen approved these changes

@Pytal Pytal Pytal approved these changes

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@julien-nc julien-nc Awaiting requested review from julien-nc

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@artonge artonge Awaiting requested review from artonge

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish

Projects

None yet

Milestone

Nextcloud 23

Development

Successfully merging this pull request may close these issues.

NC 22.1.0 - Search modal

6 participants

@CarlSchwan @szaimen @kesselb @Pytal @skjnldsv