-
Notifications
You must be signed in to change notification settings - Fork 85
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
Main menu and tool code refactoring #656
Main menu and tool code refactoring #656
Conversation
TLM/TLM/U/BaseUButton.cs
Outdated
playAudioEvents = true; | ||
} | ||
|
||
public abstract bool Active { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: My road selection also provides an optional disabled state:
https://github.com/kianzarrin/Cities-Skylines-Traffic-Manager-President-Edition/blob/cb19aa340e2f6e22e24f1bca015a0191ea84ba80/TLM/TLM/UI/LinearSpriteButton.cs#L27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this entire thing is convoluted, i want a more generic button with all these states, now also including the disabled state. Will have remember to take your change in too, whether your or mine gets merged first.
Another state for buttons that might be useful is some sort of "highlight" effect? Currently there's no visual feedback to users as to what bulk applicator tools are applying; usually it is the currently selected tool + one or more other tools. For example, with priority signs tool selected, I can Ctrl+Shift+Click to apply priority signs, junction restrictions and lane arrows down a route. Would be nice if the toolbar buttons of those tools could light a different colour (eg. light blue bg to match the selection shape on map) letting me know I'm about to apply multiple tools. At some point in future, it would also be nice to toggle persistent overlays by right-clicking buttons so maybe also an "overlay" state? |
I like the ideas but not going to do any of that until a clear state diagram is created. |
59ccc24
to
aae9659
Compare
TLM/TLM/UI/MainMenu/MainMenuPanel.cs
Outdated
const int numRows = 2; | ||
const int numCols = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rework, move constants
587ae89
to
1e7777d
Compare
The |
11 is small and easy to remove. Reason it is here, because i wanted the button to look slightly different from pre-11, to spark interest in those who still run v10 and didn't pay attention that TMPE has been upgraded to 11. One idea would be to take whatever we have for logo? The chirper and traffic light? And take away the crown. And another idea was to add version to it. |
@@ -49,11 +52,11 @@ public class RemoveCitizenInstanceButtonExtender : MonoBehaviour { | |||
return button; | |||
} | |||
|
|||
public class RemoveCitizenInstanceButton : LinearSpriteButton { | |||
public class RemoveCitizenInstanceButton : BaseUButton { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double check this still works and the icon is visible.
@@ -57,11 +60,11 @@ var publicTransportVehicleInfoPanel | |||
return button; | |||
} | |||
|
|||
public class RemoveVehicleButton : LinearSpriteButton { | |||
public class RemoveVehicleButton : BaseUButton { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double check this works too, and the button is visible
Scale slider works as intended, saves position. looks good 👍 |
Updated now:
|
@aubergine10 |
Possible. |
I think with the hot reload it doesn't matter too much as only devs see that. Maybe min 5 icon width for top row would be better (assuming that fits STABLE version label)? |
I'll go with your original idea, didn't notice you had more complex logic there than just 4 button count, it was 4 buttons on top row which should be enough to fit version label. |
I've made a layout class which handles button placement and row breaks, it is not tested, i am at work and have no way to run the game. If you wish to play the happy debugging, you can take it now, otherwise i welcome you to try this branch later tonight. |
Tested in-game - the button rows/cols is working great. However the panel backgorund is glitching. Panel background didn't update when adding 5th button: At 8 buttons added it looks like panel background is being extended wrong way (or think there's extra row?) 9 buttons and panel height is correct but width still only 4 buttons wide: 10 buttons, panel bg too high again: etc... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it 👍 resizing works as described (missing translation keys are included in #723 ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working really great now - LGTM 👍
What does
Affects Main TMPE button, and main TMPE panel
More details
BaseUI
andUIBase
renamed toModUI
(class) andModUi
(field) representing root object of user interface for the TMPE mod;TrafficManager.U
namespace (BaseUButton
from oldLinearSpriteButton
)BaseUButton
andButtonTexture
going to generalize textures handling fromUIMainMenuButton
BaseUButton
renamed toGet*
functions instead of just nouns.This is code cleanup and preparation for new main menu panel design.