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

fix toggle overlapping other stuff #4134

Merged
merged 4 commits into from May 23, 2023
Merged

fix toggle overlapping other stuff #4134

merged 4 commits into from May 23, 2023

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented May 17, 2023

Signed-off-by: Simon L <szaimen@e.mail.de>
@szaimen szaimen added bug Something isn't working 3. to review Waiting for reviews regression Regression of a previous working feature design Design, UX, interface and interaction design labels May 17, 2023
@szaimen szaimen added this to the 8.0.0 milestone May 17, 2023
@szaimen
Copy link
Contributor Author

szaimen commented May 17, 2023

/backport to stable7

Copy link
Member

@Jerome-Herbinet Jerome-Herbinet left a comment

Choose a reason for hiding this comment

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

LGTM after a fast test :
Capture vidéo du 17-05-2023 14:10:04.webm

May some other reviewers test it ?

@ChristophWurst ChristophWurst removed their request for review May 17, 2023 12:19
@GretaD
Copy link
Contributor

GretaD commented May 17, 2023

if this is the result we want, then works for me too
Screenshot from 2023-05-17 15-07-28

@raimund-schluessler
Copy link
Contributor

if this is the result we want, then works for me too Screenshot from 2023-05-17 15-07-28

I would tend to say that the collapse button should be on the very right:
Screenshot 2023-05-17 at 15-18-59 Nextcloud Vue Style Guide

Minor reordering of the elements would make this work.

@szaimen
Copy link
Contributor Author

szaimen commented May 17, 2023

Minor reordering of the elements would make this work.

Can you submit a patch? I am not sure where to adjust this...

@raimund-schluessler
Copy link
Contributor

Minor reordering of the elements would make this work.

Can you submit a patch? I am not sure where to adjust this...

I can try, but I don't have a dev environment at hand right now.

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented May 17, 2023

Minor reordering of the elements would make this work.

Can you submit a patch? I am not sure where to adjust this...

Should work now. See the enhanced example in the docs. I didn't test it in an app though.

@szaimen
Copy link
Contributor Author

szaimen commented May 17, 2023

Minor reordering of the elements would make this work.

Can you submit a patch? I am not sure where to adjust this...

Should work now. See the enhanced example in the docs. I didn't test it in an app though.

Thanks a lot! :)

Comment on lines -864 to +920
//shows the triangle button
// hides the triangle button
.icon-collapse {
visibility: hidden;
display: none;
}
&.app-navigation-entry--no-icon,
&:hover, &:focus {
a .app-navigation-entry-icon {
visibility: visible;
}
.icon-collapse {
//shows the triangle button
visibility: visible;
// shows the triangle button
display: initial;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think one of this changes now reverted the idea of #4103...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The dropdown arrow isn't visible at the moment either. I just had to switch from visibility to display as otherwise the button takes space even if it's not visible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see

Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Like noted in the other PRs: It would be good if the arrow is pointing down also in closed state.
It pointing to the right is unexpected – pointing down in both states would make it look like a standard HTML select element.

@raimund-schluessler
Copy link
Contributor

Like noted in the other PRs: It would be good if the arrow is pointing down also in closed state. It pointing to the right is unexpected – pointing down in both states would make it look like a standard HTML select element.

I wouldn't say it's entirely unexpected. Even Google still uses this pattern in their apps sometimes (e.g. Googlemail Web). But they indeed seem to switch to an up/down arrow for open/closed state, e.g. in their expandable list view:
https://m3.material.io/components/lists/guidelines#f6fe9c4e-7498-420a-b820-d12a2b59fc99
unnamed

So, how about we do this as well? Menu collapsed: arrow down, Menu open: arrow up?

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler
Copy link
Contributor

So, how about we do this as well? Menu collapsed: arrow down, Menu open: arrow up?

I implemented it like this. Please have a look.

Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Yep, looks good now!

@szaimen
Copy link
Contributor Author

szaimen commented May 23, 2023

The dropdown icon when collapsed is not shown permanently:

without hover with hover
image image

cc @Jerome-Herbinet

@raimund-schluessler
Copy link
Contributor

The dropdown icon when collapsed is not shown permanently:
without hover with hover
image image

cc @Jerome-Herbinet

How about we simply show it all the time (when collapsed and open). I would find it confusing to only show it when collapsed and hide it when open.

@szaimen
Copy link
Contributor Author

szaimen commented May 23, 2023

How about we simply show it all the time (when collapsed and open). I would find it confusing to only show it when collapsed and hide it when open.

Yes, it should be shown all the time :)

@skjnldsv skjnldsv merged commit d05d116 into master May 23, 2023
16 checks passed
@skjnldsv skjnldsv deleted the enh/noid/fix-toggle branch May 23, 2023 13:40
.icon-collapse {
visibility: hidden;
display: none;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@raimund-schluessler can the above be addressed by simply setting this to display:initial?

@Pytal Pytal mentioned this pull request Jun 6, 2023
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 regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants