Page MenuHomeWildfire Games

[gui] combine attack tooltips
ClosedPublic

Authored by Nescio on Sep 3 2020, 12:08 PM.

Details

Reviewers
bb
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP24022: Indent attack tooltips
Summary

This patch visually groups the attack tooltips together by starting with an "Attack:" line and then inserts a four-spaces indentation for each individual tooltip, as is already done for the resistance tooltip (D2229/rP24001), to improve readability.
See also D2247 and D2985.

[EDIT] Also merges getSplashDamageTooltip with getAttackTooltip, which saves lines of code.

Before:


After:

Before:

After:

Before:

After:

Test Plan

Verify everything works as intended.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Nescio created this revision.Sep 3 2020, 12:08 PM
Owners added a subscriber: Restricted Owners Package.Sep 3 2020, 12:08 PM
Nescio requested review of this revision.Sep 3 2020, 12:13 PM
Nescio edited the summary of this revision. (Show Details)Sep 3 2020, 12:13 PM
bb added a subscriber: bb.Sep 3 2020, 12:35 PM

Seems nice overall indeed. I do would like the splash issue to be fixed before merging.

binaries/data/mods/public/gui/common/tooltips.js
394 ↗(On Diff #13371)

Was about to yell about it ;)

Nescio updated this revision to Diff 13374.Sep 3 2020, 1:00 PM
  • merge splash damage into attack tooltip
Nescio edited the summary of this revision. (Show Details)Sep 3 2020, 1:05 PM
bb updated this revision to Diff 13386.Sep 3 2020, 9:06 PM

A proposal: indent splash and statuseffects under the attacktype

Looks good as is. (Until the tooltips are overhauled.)

binaries/data/mods/public/gui/common/tooltips.js
412 ↗(On Diff #13386)

I think you'd want the "\n " not here but rather above (L387)?

Shouldn't the splashDetails be moved up, to e.g. line 320?

binaries/data/mods/public/gui/common/tooltips.js
387 ↗(On Diff #13386)

Perhaps list "splash" before "statusEffects"?

We don't often move function around just for the sake of it. It makes it harder to track who changed what.

bb added a comment.Sep 4 2020, 4:42 PM

Note that internationalization is as broken as before (rP22754)

Shouldn't the splashDetails be moved up, to e.g. line 320?

yup, we should. Since we change the nature of the function totally, we should also move it to the correct place.

@Nescio want to finalize the patch?

binaries/data/mods/public/gui/common/tooltips.js
387 ↗(On Diff #13386)

can make sense

412 ↗(On Diff #13386)

maybe y

Nescio updated this revision to Diff 13399.Sep 4 2020, 5:16 PM
  • move splashDetails function up, per @bb
  • introduced var indent = " "; because I got fed up with counting spaces
Freagarach added inline comments.Sep 4 2020, 5:23 PM
binaries/data/mods/public/gui/common/tooltips.js
771 ↗(On Diff #13399)

(Unrelated.)

binaries/data/mods/public/gui/reference/common/ReferencePage.js
55 ↗(On Diff #13399)

Why the move?

Nescio added inline comments.Sep 4 2020, 5:29 PM
binaries/data/mods/public/gui/common/tooltips.js
771 ↗(On Diff #13399)

(Just for indentation consistency; three tabs is preferable to two tabs + seven spaces.)

binaries/data/mods/public/gui/reference/common/ReferencePage.js
55 ↗(On Diff #13399)

For consistency, see selection_details.js lines 315 to 318 and selection_panels lines 979 to 984 (below).

bb accepted this revision.Sep 4 2020, 10:15 PM

Note that some tooltips are wrapped to the next line (towers, siege tower), but it is not introduced, nor made worse by this patch

binaries/data/mods/public/gui/common/tooltips.js
11 ↗(On Diff #13399)

Globals should consistently be named by g_Foo: g_Indent

771 ↗(On Diff #13399)

Actually not true: the spaces are there to align the lines, not put indent.

This revision is now accepted and ready to land.Sep 4 2020, 10:15 PM
This revision was automatically updated to reflect the committed changes.

Thank you for the review, @bb!
Are you willing to do D2247, D2568, D2623, D2926, and D2985 too? Those are much smaller gui patches.