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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Indicate current layout mode in the layout flyout on the status bar #3159

Merged
merged 4 commits into from
Jan 18, 2021

Conversation

lukeblevins
Copy link
Contributor

image

This should work properly when layout mode is changed or on navigations. My main issue is likely code quality, so please look for any regressions. Warning: hacks are in use 馃槃

Fixes #1023

@yaira2
Copy link
Member

yaira2 commented Jan 18, 2021

@duke7553 Everything looks good! LGTM

@yaira2 yaira2 added the ready to merge Pull requests that are approved and ready to merge label Jan 18, 2021
@yaira2 yaira2 changed the title Indicate current layout mode in the flyout Indicate current layout mode in the layout flyout on the status bar Jan 18, 2021
@mdtauk
Copy link

mdtauk commented Jan 18, 2021

Should the layout button itself, display the icon of the currently chosen layout mode?

@yaira2
Copy link
Member

yaira2 commented Jan 18, 2021

Should the layout button itself, display the icon of the currently chosen layout mode?

That was one of the options we considered decided against since the flyout now includes options besides for changing the layout mode.

@mdtauk
Copy link

mdtauk commented Jan 18, 2021

Should the layout button itself, display the icon of the currently chosen layout mode?

That was one of the options we considered decided against since the flyout now includes options besides for changing the layout mode.

I wasn't saying only indicate on the status bar button, I mean in addition to the toggled style in the flyout itself.

Like how a ComboBox displays the current text in the control, as well as highlighted in the flyout.

@yaira2
Copy link
Member

yaira2 commented Jan 18, 2021

@mdtauk I am not sure I understand your proposal correctly, do you mind coming up with a rough sketch of your idea?

@yaira2 yaira2 merged commit f602cfc into main Jan 18, 2021
@yaira2 yaira2 deleted the layout-selection branch January 18, 2021 20:36
@mdtauk
Copy link

mdtauk commented Jan 18, 2021

@mdtauk I am not sure I understand your proposal correctly, do you mind coming up with a rough sketch of your idea?

image

@yaichenbaum Here you can see the icon is different between the two panes. Pressing the button would still bring up the new flyout

@yaira2
Copy link
Member

yaira2 commented Jan 18, 2021

@mdtauk Ok, that's what I originally thought you meant 馃槃. The issue with that is that it might confuse users if the icon keeps changing, it's one thing if there is an icon to indicate the layout mode, but it's another to keep changing the icon they are meant to use to trigger some actions.

@mdtauk
Copy link

mdtauk commented Jan 18, 2021

@mdtauk Ok, that's what I originally thought you meant 馃槃. The issue with that is that it might confuse users if the icon keeps changing, it's one thing if there is an icon to indicate the layout mode, but it's another to keep changing the icon they are meant to use to trigger some actions.

The tooltip would still say Layout, and the existing icon is not immediately obvious as to it's purpose - but I am just making the suggestion.

@yaira2
Copy link
Member

yaira2 commented Jan 18, 2021

@mdtauk The tooltip is good, but users still connect the icon to the action. That's also why it doesn't really matter what the icon is, users will learn what the icon does the first time that they click on it. Yes, they might eventually learn that the icon indicates the layout mode when the icon keeps changing, but I think it's better to reduce any potential confusion here.

@mdtauk
Copy link

mdtauk commented Jan 18, 2021

@mdtauk The tooltip is good, but users still connect the icon to the action. That's also why it doesn't really matter what the icon is, users will learn what the icon does the first time that they click on it. Yes, they might eventually learn that the icon indicates the layout mode when the icon keeps changing, but I think it's better to reduce any potential confusion here.

I would argue the individual view mode icons better represent the layout, but I wont belabour the point :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indicate the current layout mode
3 participants