#4033 closed defect (fixed)
[PATCH] Siege Engines are not shown in the summary
Reported by: | Imarok | Owned by: | Mate-86 |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 21 |
Component: | UI & Simulation | Keywords: | simple patch |
Cc: | Patch: |
Description (last modified by )
Attachments (10)
Change History (28)
comment:1 by , 8 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
by , 8 years ago
Attachment: | captured-units.png added |
---|
by , 8 years ago
Attachment: | captured-building.png added |
---|
by , 8 years ago
Attachment: | military-score.png added |
---|
comment:3 by , 8 years ago
Keywords: | rfc patch added |
---|---|
Summary: | Siege Engines are not shown in the summary → [PATCH] Siege Engines are not shown in the summary |
comment:4 by , 8 years ago
Description: | modified (diff) |
---|---|
Milestone: | Backlog → Alpha 21 |
by , 8 years ago
Attachment: | units-summary-fixed.png added |
---|
comment:5 by , 8 years ago
Description: | modified (diff) |
---|
follow-up: 7 comment:6 by , 8 years ago
Some open points:
- not sure whether I should update some unit tests for JS?
- I've modified counter.js:calculateUnitsTeam as well but I don't know how to test it. Should I setup a match with teams?
- There is a slight misalignment on my latest screenshot. I may need help how to align the counters to the center.
comment:7 by , 8 years ago
Replying to Mate-86:
not sure whether I should update some unit tests for JS?
We don't have tests for the GUI yet and it's not the purpose of this ticket to lie the foundation to that.
I've modified counter.js:calculateUnitsTeam as well but I don't know how to test it. Should I setup a match with teams?
Yes
There is a slight misalignment on my latest screenshot. I may need help how to align the counters to the center.
Been there, #3351, nuke the trailing newline probably.
by , 8 years ago
Attachment: | siege-units-summary.2.patch added |
---|
comment:8 by , 8 years ago
I've uploaded a new patch. Removing the newline has solved the problem but I could not check the team summary page even if I set up some teams. If no objection I'd move the ticket to review queue.
comment:10 by , 8 years ago
Thx, now I get the team summary as well. Need to apply some further fixes tomorrow.
by , 8 years ago
Attachment: | team-summary.png added |
---|
by , 8 years ago
Attachment: | siege-units-summary.3.patch added |
---|
comment:11 by , 8 years ago
Description: | modified (diff) |
---|
Team summary fixed as well, screenshot and latest patch attached.
comment:12 by , 8 years ago
counters.js L235-6 trailing whitespace + 1 newline is enough. L240, L243, L252, L254, L259: trailing whitespace
StatisticsTracker.js L352: trailing whitespace L355 indentation error
When the values start to get bigger e.g. get more digits the 4 values does not fit anymore on the one line. The lost vale will then fall of the line to the next line but it is only half visible then. It might be nice to split out the values over different lines as is done in the structure tab.
by , 8 years ago
Attachment: | siege-units-summary.4.patch added |
---|
by , 8 years ago
Attachment: | teams_multiline.png added |
---|
comment:13 by , 8 years ago
Description: | modified (diff) |
---|
comment:14 by , 8 years ago
Description: | modified (diff) |
---|
comment:15 by , 8 years ago
New solution (screenshot + patch) uploaded. Whitespace issues should be fixed as well.
comment:16 by , 8 years ago
Keywords: | review added; rfc removed |
---|
counters.js L251 trailing whitespace
Further is ok so pushing to review queue.
feedback from bb_: