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

Overhaul settings panel layout #19677

Merged
merged 7 commits into from
Jan 4, 2022

Conversation

dragunoff
Copy link
Contributor

@dragunoff dragunoff commented Sep 22, 2021

This PR lays out the settings panel in a consistent and simple way whilst accommodating any number of any type of settings widgets.

Closes #18952
Closes #18426
Related #16596

RA, D2k, TS

The vertical tabs are a precedent but I think they fit and this layout allows for more tabs to be added. In the "common" UI I decided not to touch it as there isn't enough horizontal space because of the main menu. The settings panels is made larger and is the size of the asset browser.
Screenshot_20210922_195713

TD

In TD the vertical vs horizontal tabs dissonance was more apparent so I switched the game info to also use the new vertically stacked layout. And both panels are now the same size.
Screenshot_20210922_172916
Settings panel with vertical tabs

Screenshot_20210922_172900
Game info panel with vertical tabs

@pchote
Copy link
Member

pchote commented Sep 26, 2021

I generally like this, but there are a small number of sharp edges that we should try to polish:

  • The group header inside the scroll panel for Audio and Input are redundant and IMO would be better if removed
  • The sliders, dropdowns, and hotkeys look good in a single column, but I feel that most (all?) of the checkboxes would be better arranged with sharing two columns
  • The control scheme description text feels awkward - maybe moving it right so it is clearly indented below the option would help?
  • The frame limiter checkbox and slider are really awkward too (the slider modifies the checkbox that is above it)
  • Can we please reduce the vertical stride of the TD tabs from 50 to 45? 50 feels noticeably wrong to me, 45 seems better and also matches the horizontal button separation we use in other places

@dragunoff
Copy link
Contributor Author

The group header inside the scroll panel for Audio and Input are redundant and IMO would be better if removed

Agreed.

The sliders, dropdowns, and hotkeys look good in a single column, but I feel that most (all?) of the checkboxes would be better arranged with sharing two columns

I'm not against arranging controls in columns in principle but I think that our settings panels are too narrow for that. I'd stick with a single column.

The control scheme description text feels awkward - maybe moving it right so it is clearly indented below the option would help?

I think that it fits better to the left, putting it below the dropdown feels more awkward:
Screenshot_20210928_111603

The frame limiter checkbox and slider are really awkward too (the slider modifies the checkbox that is above it)

Yes, this bugs me too. I explored two approaches:

Enable/disable the silder:
https://user-images.githubusercontent.com/1355810/135052112-f385724d-7d06-4b1b-b028-03ae94a7bbcc.mp4

Show/hide the slider:
https://user-images.githubusercontent.com/1355810/135052130-7c97ed43-eb0b-44c7-8587-87697f9271bf.mp4

Can we please reduce the vertical stride of the TD tabs from 50 to 45? 50 feels noticeably wrong to me, 45 seems better and also matches the horizontal button separation we use in other places

Agreed.

@pchote
Copy link
Member

pchote commented Sep 28, 2021

The thing that bothers me with the checkboxes is that all the other input types focus their input/attention to the right of the panel, but checkboxes focuses it on the far left, making those tabs feels disjointed.

My first thought was that we could mitigate this by having two checkboxes per row, so at least we have some kind of content in the right half of those rows.

My second thought is that we could instead align the checkboxes to match the dropdowns and sliders:

Screenshot 2021-09-28 at 09 58 53

This is only a problem where there are checkboxes sandwiched between dropdowns, so the advanced panel could keep its checkboxes left aligned. This screenshot also shows a bug: changing the UI Scale makes all the tabs collapse on themselves - DisplaySettingsLogic.RecalculateWidgetLayout will need a hack to skip these like it does for badges.

A more radical option could be to place the labels to the left:

Screenshot 2021-09-28 at 10 12 31

But i don't like how this leaves only a small visible click target to toggle the option.

@dragunoff
Copy link
Contributor Author

Alright, I see and I agree that the layout can be improved. How about right-aligning all labels:
image
This has the benefit of bringing the labels closer to the controls. Checkboxes of this kind are a precedent in the OpenRA UI but I don't think they look too out of place. To improve the usability we can make the labels trigger the control that they are describing (like the for attribute in HTML).

An alternative is to make the panel wider, place all labels (except on checkboxes) above the controls and go all out 2-column. Roughly like this screen from BroodWar:
image

@pchote
Copy link
Member

pchote commented Sep 28, 2021

The broodwar approach could potentially work well and IMO is definitely worth at least mockup in our common/TD UI. We could do the same for the lobby options bin, which would let us use 3 columns for dropdowns instead of mixing 2 and 3 columns in the UI.

@dragunoff dragunoff force-pushed the feature/settings-layout-rework branch from eff08ce to 5586314 Compare October 5, 2021 17:53
@dragunoff
Copy link
Contributor Author

Layout changed to 2-columns:
Screenshot_20210929_164708
Screenshot_20210929_165000

Also updated the introductory prompts to match:
Screenshot_20210929_205411
Screenshot_20210929_210730

And a few other things:

  • I added the tab container to the hack in DisplaySettingsLogic.RecalculateWidgetLayout.e
  • Also fixed a bug with the disabled state of the slider thumb.
  • And finally I removed the redundant "This is the default" message for hotkeys. If this is not desired I will drop it.

@dragunoff
Copy link
Contributor Author

I have some more tweaking to do here so please hold off reviewing for a little while.

@dragunoff dragunoff force-pushed the feature/settings-layout-rework branch from 5586314 to 82c61b7 Compare October 7, 2021 08:30
@dragunoff
Copy link
Contributor Author

dragunoff commented Oct 7, 2021

This is now ready for review.

Updated common game ingame info tabs to match settings layout
Screenshot-20211007112652-1282x807

Updated lobby options tab layout to match settings layout
Screenshot-20211007112556-1282x807

@dragunoff
Copy link
Contributor Author

dragunoff commented Oct 7, 2021

Update: changed how the settings widget widths and positions are calculated (in YAML). This doesn't alter the appearance, just makes things a little more robust.

@anvilvapre
Copy link
Contributor

Would there be room for a checkbox for GameSettings.EnableDiscordService #19343?

@dragunoff
Copy link
Contributor Author

EnableDiscordService

Yes, the new layout can accommodate any number of settings widgets. A checkbox for discord seems like a sensible feature but should be added in a follow up PR.

@dragunoff dragunoff force-pushed the feature/settings-layout-rework branch from 4092819 to 5f76f5f Compare November 1, 2021 13:41
@dragunoff
Copy link
Contributor Author

Rebased.

@dragunoff dragunoff force-pushed the feature/settings-layout-rework branch from 5f76f5f to fe37960 Compare December 29, 2021 13:09
@dragunoff
Copy link
Contributor Author

Rebased after the merge of #19681 so it all comes together.

@pchote
Copy link
Member

pchote commented Dec 29, 2021

The darker separator style in RA and (to a lesser extent) TS still feel weird to me, but I imagine i'll get used to them if they go ahead. I do object to these separators having a different style to the separators used in the mission and multiplayer browsers, though. Can we please switch those to the same style, or keep everything using the old style here and open a new PR to update the separators in all four places as a followup?

@pchote
Copy link
Member

pchote commented Dec 29, 2021

Perhaps best in a followup PR, but IMO good polish to fix before a release: The labels for the lobby options dropdown buttons should end with a colon to match the settings menu, and be greyed out when disabled (in the ingame menu / for non-admins in the lobby).

Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@dragunoff
Copy link
Contributor Author

The darker separator style in RA and (to a lesser extent) TS still feel weird to me, but I imagine i'll get used to them if they go ahead. I do object to these separators having a different style to the separators used in the mission and multiplayer browsers, though. Can we please switch those to the same style, or keep everything using the old style here and open a new PR to update the separators in all four places as a followup?

Agreed. It bugs me too. Let's keep the old style here and defer the new style to a follow up.

Perhaps best in a followup PR, but IMO good polish to fix before a release: The labels for the lobby options dropdown buttons should end with a colon to match the settings menu, and be greyed out when disabled (in the ingame menu / for non-admins in the lobby).

Good catch. I'll add the colons here but leave the greying out to a follow up as it's not so straight forward - we need to establish a link between the label and the dropdown.

Thanks for the review @pchote.

@dragunoff dragunoff force-pushed the feature/settings-layout-rework branch from fe37960 to d225886 Compare December 30, 2021 11:38
@dragunoff
Copy link
Contributor Author

Update:

  • switched the separator to the old style (button)
  • added colons after the labels of lobby dropdowns

Width: PARENT_RIGHT
Height: 20
Font: Regular
Text: UI Feedback in Transients Panel
Copy link
Contributor

Choose a reason for hiding this comment

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

@pchote not entirely sure, but shouldn't this be Transient Panels?

Copy link
Member

Choose a reason for hiding this comment

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

I think Transients Panel is indeed correct, since this is referring to the new ingame display for transient feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does sound somewhat unclear. Should we change to something like "Show UI Feedback Notifications"? Or add a tooltip description? (not necessarily in this PR)

Copy link
Member

Choose a reason for hiding this comment

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

Show UI Feedback Notifications

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, but I'll merge this first then (there are some other improvements I'd like to see as well, I'll file a follow-up PR or issue about it later).

@reaperrr reaperrr merged commit c3dfac7 into OpenRA:bleed Jan 4, 2022
@dragunoff dragunoff deleted the feature/settings-layout-rework branch January 4, 2022 17:36
@reaperrr
Copy link
Contributor

reaperrr commented Jan 4, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overhaul settings panel layout Reorder the Display tab in Settings
5 participants