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

Files : Left panel dropdown improvement for NC 26 #4103

Merged

Conversation

Jerome-Herbinet
Copy link
Member

@Jerome-Herbinet Jerome-Herbinet commented May 12, 2023

(from @Clementine46)

/!\ Testing is welcome in order to make sure it's not doing side effects elsewhere in the UI.

Before :

before.webm

After :

after.webm

Signed-off-by: Jérôme Herbinet <[email protected]>

Files : Left panel dropdown improvement

Signed-off-by: Jérôme Herbinet <[email protected]>
@Jerome-Herbinet
Copy link
Member Author

Also see the other PR for Nextcloud 25 nextcloud/server#38217

Copy link

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Super nice! I would only suggest that the hover effect doesn't need to use the primary color as we don't use that pattern anywhere else yet, the hover feedback can just be the change in the background color :) other than that it looks great!

@Jerome-Herbinet
Copy link
Member Author

Super nice! I would only suggest that the hover effect doesn't need to use the primary color as we don't use that pattern anywhere else yet, the hover feedback can just be the change in the background color :) other than that it looks great!

Thanks for the comment @nimishavijay ; can you just give me more details about what hover effect you are talking about. I'm not sure to understand ... or can you join and illustrated screenshot ? (there are 2 hover effects that I didn't change : the one under the dropdown arrow and the one under the whole button).

@Jerome-Herbinet Jerome-Herbinet added the 3. to review Waiting for reviews label May 12, 2023
@nimishavijay
Copy link

I meant the hover effect under the dropdown button icon, on hover the icon changes to the primary color.

image

I just checked the other apps, and you are right, nothing has been changed by you. However, it's not intended in the first place. So I suggest on hover we could just have the background color changing, and the color of the icon can remain the same (--color-main-text). What do you think? :)

Signed-off-by: Jérôme Herbinet <[email protected]>
@Jerome-Herbinet
Copy link
Member Author

I meant the hover effect under the dropdown button icon, on hover the icon changes to the primary color.

image

I just checked the other apps, and you are right, nothing has been changed by you. However, it's not intended in the first place. So I suggest on hover we could just have the background color changing, and the color of the icon can remain the same (--color-main-text). What do you think? :)

OK understood @nimishavijay :-) It's done.

Copy link

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

If the hover effect has been changed, then it is good to go! nice work! :)

@skjnldsv
Copy link
Contributor

/backport to stable7

@skjnldsv skjnldsv added bug Something isn't working design Design, UX, interface and interaction design feature: app-navigation Related to the app-navigation component and removed backport-request labels May 12, 2023
@skjnldsv skjnldsv merged commit 2d6245a into master May 12, 2023
@skjnldsv skjnldsv deleted the Jerome-Herbinet-left-panel-dropdown-improvement-patch-1 branch May 12, 2023 15:43
@GretaD
Copy link
Contributor

GretaD commented May 17, 2023

this pr breaks mail, and most probably other apps that have an actionmenu on side bar

Screenshot from 2023-05-17 12-14-29

@Jerome-Herbinet
Copy link
Member Author

this pr breaks mail, and most probably other apps that have an actionmenu on side bar

Screenshot from 2023-05-17 12-14-29

@GretaD,
Thanks for your feedback.
IMO it seems complicated to separate behaviour in nextcloud-vue to apply styles only in "Files" context (obviously, I don't find there any specific class or id to target, relative to Files context).
Instead, what do you think about trying to override CSS styles in apps.scss (core) ?
Any other suggestion ?

@nimishavijay @jancborchardt @juliushaertl

@szaimen
Copy link
Contributor

szaimen commented May 17, 2023

I think this needs to be handled by this component. If both features are used at the same time, it should not coolide but instead be shown next to each other.

@ChristophWurst
Copy link
Contributor

Apps are reaching RC state for Nextcloud 27 today. Can this be fixed quickly or should we revert and redo cleanly?

@Jerome-Herbinet
Copy link
Member Author

I think things look more delicate and complex than I thought before starting to work on it.
I propose to close this RP for now (and retry quietly later).
@szaimen @ChristophWurst

@szaimen
Copy link
Contributor

szaimen commented May 17, 2023

Let me try one thing. If that does not work we can revert this change.

@Jerome-Herbinet
Copy link
Member Author

Note also that the problem could be the same in nextcloud/server#38338 event if styles are elsewhere.

@szaimen
Copy link
Contributor

szaimen commented May 17, 2023

This fixes the problem in my testing: #4134

Let me know if it also for you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working design Design, UX, interface and interaction design feature: app-navigation Related to the app-navigation component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invisible drop down button on left panel
6 participants