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

Updating observer/spectator ui with a new layout #16406

Merged
merged 4 commits into from May 22, 2019

Conversation

@teinarss
Copy link
Contributor

commented Apr 13, 2019

Problems

  • Dk2 and TS the main menu button is at the top left position where the new spectator dropdown is.

Preview

spec2

Dependency #16485

Fixes #5121

@teinarss teinarss force-pushed the teinarss:new_observer_stats branch 2 times, most recently from 32b7ec8 to 1e9fde9 Apr 16, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

I have a few UI niggles, using these two screenshots as talking points:

Screenshot 2019-04-21 at 12 02 37

vs

Screenshot 2019-04-21 at 11 59 24

  • The gradient mismatch between the team headers and the player rows (and between different player rows when different numbers of items are being built) looks weird. Can we draw them all with the same fadeout / length?

  • There is a 1px transparent hole along the top and left edges of the player/team rows

  • The invisible scrollbar bin looks very odd. Can we please draw the bar background here?

  • Bonus points if you add a ScrollBarAlign.Hidden that hides the bar, and toggle this if the match doesn't have enough players to need to scroll.

  • When mousing over a player row in the "Basic" view we it reveals that there is no right hand margin in the fade out:
    Screenshot 2019-04-21 at 12 03 12

  • The half-sized production icons IMO still look bad, but we can address this by introducing the lowres icons in followup PRs.

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

Switching between tabs desyncs the header and player row widths:

Screenshot 2019-04-22 at 19 04 30

@teinarss teinarss force-pushed the teinarss:new_observer_stats branch from d4ca02e to 6cec439 Apr 22, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

Updated!

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

This is looking really great, but still a few more things to fix!

  • For consistency IMO the two graphs should be placed inside the scrollpanel, with the bar on the left (if applicable) but disabled, the same height as the other panels, and the same width as the Basic tab.

  • The tab dropdown and info scrollpanel in RA should be using the observer grey dialog variant

  • The tab dropdown should define a larger maxheight - IMO there is no reason to hide the two graphs

  • This seems to have regressed the scrollpanel bin background for RA observers:
    Screenshot 2019-04-26 at 21 03 33

  • IMO the tab dropdown should have some kind of indication as to what it actually is instead of just saying None by default. Maybe "Information: None"?

  • Graph labels need to define a shadow - they are unreadable on snow maps:
    Screenshot 2019-04-26 at 21 07 14

  • The GradientColorBlockWidget has some rounding issues that become visible on HiDPI displays - I vaguely remember fixing these with the regular color block in the distant past.

  • Dk2 and TS the main menu button is at the top left position where the new spectator dropdown is.

    The dropdown needs to be moved to the right in these mods.

  • Faction flags are offset incorrectly in D2k and TS:

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

* For consistency IMO the two graphs should be placed inside the scrollpanel, with the bar on the left (if applicable) but disabled, the same height as the other panels, and the same width as the Basic tab.

I did think it looked a bit strange with the scroll for the graphs and it also solved a bug when i removed them from the scrollpanel which i need to fix if we gonna move them back there! :P
And by the same height you mean not the visual height but instead the height of the scrollpanel - margin for the graph?

@teinarss teinarss force-pushed the teinarss:new_observer_stats branch 2 times, most recently from da7e26f to 57f0bd1 Apr 27, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Updated

@teinarss teinarss force-pushed the teinarss:new_observer_stats branch from 57f0bd1 to 6c7f162 May 2, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented May 10, 2019

#16485 has been merged, so needs a rebase.

@teinarss teinarss force-pushed the teinarss:new_observer_stats branch from 6c7f162 to c843cf8 May 10, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Rebased! How do we want to handle the showing of the Support Powers in the top right corner in RA?

@pchote pchote removed the PR: Rebase me! label May 10, 2019

@teinarss teinarss force-pushed the teinarss:new_observer_stats branch 2 times, most recently from 95849b5 to e19978d May 11, 2019

@teinarss teinarss force-pushed the teinarss:new_observer_stats branch from 28f44f5 to 9de4ef2 May 21, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Updated fixed the hover effect on the player rows

@dragunoff

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

Perhaps the colored rows approach is not that good fundamentally. It looks flashy and distracting. I think this should be more subtle.

A quick mock as a starting point:
изображение

  • In the normal state the player color is visible only in the name
  • The row is colored only on hover and then the player name also changes to white

In addition:

  • The rows could be made in alternating shades (zebra table)
  • The hover could be made as a gradient instead of a solid block
@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented May 22, 2019

If we really do want to start reconsidering major design points of this UI then we should defer the feature to a future release. This has already taken up a disproportionate amount of review effort that we really need elsewhere atm, and my motivation to repeat all of that for a new design with new regressions is near zero.

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

This is a long awaited and very much wanted feature. Would be a pity to defer it. Even though I raised some concerns I would agree that this is better than what we have right now. The general layout and approach here is good. The design can be amended in follow-ups.

@teinarss teinarss force-pushed the teinarss:new_observer_stats branch 2 times, most recently from abcdfcc to ae05754 May 22, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Updated and rebased

@pchote

This comment has been minimized.

Copy link
Member

commented May 22, 2019

I'm getting NREs on game start in all mods. Forgot to commit a file/change?

@teinarss teinarss force-pushed the teinarss:new_observer_stats branch from ae05754 to bae3d7d May 22, 2019

@pchote pchote added this to the Next Release milestone May 22, 2019

@pchote
pchote approved these changes May 22, 2019

@pchote pchote merged commit 9fc8b82 into OpenRA:bleed May 22, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.