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

Adjusted text v-alignment on labels, checkbox and buttons #16592

Merged
merged 9 commits into from Jun 21, 2019

Conversation

@teinarss
Copy link
Contributor

commented May 25, 2019

Closes #16530

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

@teinarss teinarss force-pushed the teinarss:text_valign branch from 5a1fb45 to 7bc610d May 25, 2019

@pchote
Copy link
Member

left a comment

There are still quite a few places where the offsets differ (IMO for the worse).

To pick a few examples:
menu
lobby1
lobby2
lobby3
mission

OpenRA.Mods.Common/Widgets/ButtonWidget.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/CheckboxWidget.cs Outdated Show resolved Hide resolved
@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented May 25, 2019

IMO the buttons etc are still slightly, maybe 1px, too high. Likewise for the badge icons.

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

Before the buttons had a margin of 12px top and 9px bottom, now they are 10 px and 11px. Maybe it would look even better with 11px and 10px. We had the same problem with the label when it was valigned top that it was slightly outside of the label.

@teinarss teinarss force-pushed the teinarss:text_valign branch 3 times, most recently from dc37672 to e69c39d May 26, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

Updated but looks like need to do a rebase because of fde2153 ughh

@teinarss teinarss force-pushed the teinarss:text_valign branch from e69c39d to abccb04 May 30, 2019

@ltem

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

The Travis CI build failed.

Travis CI Log:

$ make test
  OpenRA.PostProcess -> /home/travis/build/OpenRA/OpenRA/OpenRA.PostProcess.exe
  OpenRA.Game -> /home/travis/build/OpenRA/OpenRA/OpenRA.Game.exe
  OpenRA.Mods.Common -> /home/travis/build/OpenRA/OpenRA/mods/common/OpenRA.Mods.Common.dll
  OpenRA.Mods.Cnc -> /home/travis/build/OpenRA/OpenRA/mods/common/OpenRA.Mods.Cnc.dll
  OpenRA.Utility -> /home/travis/build/OpenRA/OpenRA/OpenRA.Utility.exe
  OpenRA.Server -> /home/travis/build/OpenRA/OpenRA/OpenRA.Server.exe
  OpenRA.Mods.D2k -> /home/travis/build/OpenRA/OpenRA/mods/d2k/OpenRA.Mods.D2k.dll
  OpenRA.Platforms.Default -> /home/travis/build/OpenRA/OpenRA/OpenRA.Platforms.Default.dll
  OpenRA.Test -> /home/travis/build/OpenRA/OpenRA/OpenRA.Test.dll
Testing Tiberian Sun mod MiniYAML...
Testing mod: Tiberian Sun
OpenRA.Utility(1,1): Error: Failed with exception: System.AggregateException: One or more errors occurred. (Object reference not set to an instance of an object) ---> System.NullReferenceException: Object reference not set to an instance of an object
  at OpenRA.Mods.Common.Traits.Render.WithTextControlGroupDecorationInfo.OpenRA.Traits.IRulesetLoaded<OpenRA.ActorInfo>.RulesetLoaded (OpenRA.Ruleset rules, OpenRA.ActorInfo info) [0x00000] in /home/travis/build/OpenRA/OpenRA/OpenRA.Mods.Common/Traits/Render/WithTextControlGroupDecoration.cs:44 
  at OpenRA.Ruleset..ctor (OpenRA.IReadOnlyDictionary`2[TKey,TValue] actors, OpenRA.IReadOnlyDictionary`2[TKey,TValue] weapons, OpenRA.IReadOnlyDictionary`2[TKey,TValue] voices, OpenRA.IReadOnlyDictionary`2[TKey,TValue] notifications, OpenRA.IReadOnlyDictionary`2[TKey,TValue] music, OpenRA.TileSet tileSet, OpenRA.Graphics.SequenceProvider sequences, OpenRA.IReadOnlyDictionary`2[TKey,TValue] modelSequences) [0x00072] in /home/travis/build/OpenRA/OpenRA/OpenRA.Game/GameRules/Ruleset.cs:59 
  at OpenRA.Ruleset+<>c__DisplayClass12_0.<LoadDefaults>b__0 () [0x00196] in /home/travis/build/OpenRA/OpenRA/OpenRA.Game/GameRules/Ruleset.cs:151 
  at System.Threading.Tasks.Task.InnerInvoke () [0x0000f] in <6649516e5b3542319fb262b421af0adb>:0 
  at System.Threading.Tasks.Task.Execute () [0x00000] in <6649516e5b3542319fb262b421af0adb>:0 
   --- End of inner exception stack trace ---
  at System.Threading.Tasks.Task.ThrowIfExceptional (System.Boolean includeTaskCanceledExceptions) [0x00011] in <6649516e5b3542319fb262b421af0adb>:0 
  at System.Threading.Tasks.Task.Wait (System.Int32 millisecondsTimeout, System.Threading.CancellationToken cancellationToken) [0x00043] in <6649516e5b3542319fb262b421af0adb>:0 
  at System.Threading.Tasks.Task.Wait (System.Int32 millisecondsTimeout) [0x00000] in <6649516e5b3542319fb262b421af0adb>:0 
  at OpenRA.Ruleset.LoadDefaults (OpenRA.ModData modData) [0x00075] in /home/travis/build/OpenRA/OpenRA/OpenRA.Game/GameRules/Ruleset.cs:162 
  at OpenRA.ModData.<.ctor>b__30_3 () [0x00000] in /home/travis/build/OpenRA/OpenRA/OpenRA.Game/ModData.cs:95 
  at System.Lazy`1[T].ViaFactory (System.Threading.LazyThreadSafetyMode mode) [0x00043] in <6649516e5b3542319fb262b421af0adb>:0 
  at System.Lazy`1[T].ExecutionAndPublication (System.LazyHelper executionAndPublication, System.Boolean useDefaultConstructor) [0x00022] in <6649516e5b3542319fb262b421af0adb>:0 
  at System.Lazy`1[T].CreateValue () [0x00074] in <6649516e5b3542319fb262b421af0adb>:0 
  at System.Lazy`1[T].get_Value () [0x0000a] in <6649516e5b3542319fb262b421af0adb>:0 
  at OpenRA.ModData.get_DefaultRules () [0x00000] in /home/travis/build/OpenRA/OpenRA/OpenRA.Game/ModData.cs:41 
  at OpenRA.Mods.Common.UtilityCommands.CheckYaml.OpenRA.IUtilityCommand.Run (OpenRA.Utility utility, System.String[] args) [0x000a2] in /home/travis/build/OpenRA/OpenRA/OpenRA.Mods.Common/UtilityCommands/CheckYaml.cs:64 
---> (Inner Exception #0) System.NullReferenceException: Object reference not set to an instance of an object
  at OpenRA.Mods.Common.Traits.Render.WithTextControlGroupDecorationInfo.OpenRA.Traits.IRulesetLoaded<OpenRA.ActorInfo>.RulesetLoaded (OpenRA.Ruleset rules, OpenRA.ActorInfo info) [0x00000] in /home/travis/build/OpenRA/OpenRA/OpenRA.Mods.Common/Traits/Render/WithTextControlGroupDecoration.cs:44 
  at OpenRA.Ruleset..ctor (OpenRA.IReadOnlyDictionary`2[TKey,TValue] actors, OpenRA.IReadOnlyDictionary`2[TKey,TValue] weapons, OpenRA.IReadOnlyDictionary`2[TKey,TValue] voices, OpenRA.IReadOnlyDictionary`2[TKey,TValue] notifications, OpenRA.IReadOnlyDictionary`2[TKey,TValue] music, OpenRA.TileSet tileSet, OpenRA.Graphics.SequenceProvider sequences, OpenRA.IReadOnlyDictionary`2[TKey,TValue] modelSequences) [0x00072] in /home/travis/build/OpenRA/OpenRA/OpenRA.Game/GameRules/Ruleset.cs:59 
  at OpenRA.Ruleset+<>c__DisplayClass12_0.<LoadDefaults>b__0 () [0x00196] in /home/travis/build/OpenRA/OpenRA/OpenRA.Game/GameRules/Ruleset.cs:151 
  at System.Threading.Tasks.Task.InnerInvoke () [0x0000f] in <6649516e5b3542319fb262b421af0adb>:0 
  at System.Threading.Tasks.Task.Execute () [0x00000] in <6649516e5b3542319fb262b421af0adb>:0 <---
Makefile:133: recipe for target 'test' failed
make: *** [test] Error 1
The command "make test" exited with 2.

@teinarss teinarss force-pushed the teinarss:text_valign branch from abccb04 to 2d203cf May 31, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

Rebased updated and whatnot!

@pchote
Copy link
Member

left a comment

A couple of comments from TD to start with. I'll keep working through the rest of the mods as time allows.

The CD icons in the mod content installer also need to move down by a couple of px to remain aligned with the text.

  • Code changes
  • TD UI
  • RA UI
  • D2k UI
  • TS UI
mods/cnc/chrome/settings.yaml Outdated Show resolved Hide resolved
mods/cnc/chrome/music.yaml Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

  • The ingame "Silo Usage" and "Power Usage" tooltips are one px too tall when compared with "Unrevealed terrain" (when hovering over shroud). I think we need to trim off one of the pixels below the text?

  • The support power tooltip probably looks like it might need an extra px of bottom padding(?)

mods/cnc/chrome/mainmenu.yaml Outdated Show resolved Hide resolved
@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

* The ingame "Silo Usage" and "Power Usage" tooltips are one px too tall when compared with "Unrevealed terrain" (when hovering over shroud). I think we need to trim off one of the pixels below the text?

The Silo and Power tooltip have its Text vertical centered, padding with 7px top bottom compared to the Unrevealed terrain tooltip that have 7 and 6.
My suggestion is to make the Unrevealed terrain tooltip one px higher.

@teinarss teinarss force-pushed the teinarss:text_valign branch from 2d203cf to da256c3 Jun 7, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Updated!

@pchote
Copy link
Member

left a comment

A few more:

  • Faction description tooltip bottom margin in the lobby appears to be a couple of px too large.
  • Most one-line tooltips in RA (e.g. world tooltip, button tooltip, power usage tooltip) top/bottom margins appears to be mismatched by a px or two.
  • RA support power tooltip bottom margin is too small
  • The vertical spacing above/below the author line in the RA replay browser looks wrong (existing bug, but would be good to fix here)
  • Needs a rebase to fix crash when opening the map editor
  • Vertical spacing of CD icon in the content installer still feels 1px too high, IMO.
mods/cnc/chrome/settings.yaml Outdated Show resolved Hide resolved
mods/d2k/chrome/mainmenu.yaml Outdated Show resolved Hide resolved
mods/common/chrome/mainmenu.yaml Outdated Show resolved Hide resolved

@teinarss teinarss force-pushed the teinarss:text_valign branch from da256c3 to 829eb05 Jun 8, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

Updated and rebased

@teinarss teinarss force-pushed the teinarss:text_valign branch from 829eb05 to c26518f Jun 8, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

Aside from that, I think the remaining tooltip complaints are better resolved in a followup PR. Half the problems there are because the different *TooltipLogics code make different assumptions about how to calculate the bottom margins (some use the left margin of an element, others use the top, etc).

@teinarss teinarss force-pushed the teinarss:text_valign branch from ffae2f3 to e8ff68b Jun 9, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

Updated

@teinarss teinarss force-pushed the teinarss:text_valign branch from e8ff68b to 2acec90 Jun 9, 2019

@pchote pchote added the PR: Needs +2 label Jun 10, 2019

@teinarss teinarss force-pushed the teinarss:text_valign branch from 2acec90 to 370b131 Jun 10, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Updated!

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Having this remain broken on bleed for so long is now getting silly. I'll go ahead and merge this on the weekend if we don't get a second review asking for changes before then.

@abcdefg30
Copy link
Member

left a comment

Looks good to me otherwise.

OpenRA.Mods.Common/Widgets/ChatDisplayWidget.cs Outdated Show resolved Hide resolved
mods/modcontent/content.yaml Outdated Show resolved Hide resolved

@teinarss teinarss force-pushed the teinarss:text_valign branch from 370b131 to 1568a28 Jun 21, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Updated

@abcdefg30 abcdefg30 merged commit b647d46 into OpenRA:bleed Jun 21, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

@@ -168,27 +168,35 @@ Fonts:
Tiny:
Font: common|FreeSans.ttf
Size: 10
Ascender: 8

This comment has been minimized.

Copy link
@GraionDilach

GraionDilach Jun 23, 2019

Contributor

These need to be mentioned within the ModSDK notes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.