-
Notifications
You must be signed in to change notification settings - Fork 79
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
GUIs refactor using mod-gui #1418
Conversation
Just a heads up that I probably won't be able to review this before the weekend. In the meantime it might be worth posting a before and after screenshot in the admin channel just to make sure people know what is coming. |
I pushed a change to fix the gui tests, hope that is ok. You can run the tests with the |
I don't like this change. Is this a temporary change, or is difficult to get this working? |
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.
Code looks fine to me. I tested it and didn't find any problems (apart form with snake, which no one uses anyway).
There are a few minor style changes that I think we should change, or for you to say you don't think we should change.
I will advance that all choices were made by design and are purely subjective choices, I don't have too strong feelings about keeping or changing any of these :)
That's because I added some padding to it
Yes, it was deliberate that the hide/shows button stays consistent in size as other mods buttons will not be hidden (only Redmew's) and they all share the same "cage". But I could make it shrink again if we want, it just wont matter much if other buttons are there as well
Was just a design choice. Some buttons had the highlight toggle, some didn't which was a bit inconsistent. Most of the buttons open a left window that have a "close" button on their own, so it would need to react & change the button state in multiple places for little benefit(?). Ideally, I think it would be cleaner to use the flag |
Maybe this is because of interface UI scale settings, I use 175%, but if I switch to 100% it looks like yours.
Fair enough, I wondered if it was to do with mod buttons. I think we can keep this change then.
All of the Redmew buttons that open a window had the highlighting working before, so I'm not sure what you mean by the inconsistency. I can live without the highlighting, but it was deliberate choice to put it in. |
I will add the toggle back its not a biggie, it was a feature we had, happy to maintain it if players find it useful Good catch on the UI scale! I would never have guessed. I looked at the API and didn't find anything to make it scale better with interface % (it does look a bit weird that it's not consistent to button size when changing zoom from 75% to 200% tho) |
There is |
That is possibly getting a bit too complicated. The styling issue is minor, so I'd be happy to leave it. |
@grilledham there's a couple more issues on which I'd like to hear your feedback, when using mods:
|
|
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.
LGTM. Thanks for updating our gui to use a more modern style.
Description
Base game offers
core/mod-gui.lua
as a collective system to register buttons and frames ingui.top
andgui.left
. I think it looks cleaner overall so I took the chance to renovate the whole look (note: for the theme, we could also switch to light mode and have light-grey buttons over the darker background, but I think this looks more up-to-date...)Migration
mod-gui
adds a few more nested GuiElements, soI.
gui.left
->gui.left.mod_gui_frame_flow
II.
gui.top
->gui.top.mod_gui_top_frame.mod_gui_inner_frame
overall, the lib itself takes care of ensuring that those children are always present when adding to them, so our
utils/gui.lua
lib simply relies on it and replaces all instances ofplayer.gui.top[element_name]
withGui.get_top_element(element_name)
(same for left, both withget
andadd
methods).Changes
utils/gui.lua
lib and added default styles for all top/left elementsgui.test
, in particularI hope this maintains the same logic as before? Counting the actual added children? Also
gui.screen
was not present and I did not add, don't know if that was on purposeutility/clear
spriteplayer_create.lua
and moved togui.lua
instead at same event IDTest
Here's a preview of the changes: I plan to come back and maybe refactor some of the left frames as well to make them more "factorio-like" in the future instead of plain grey windows.