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

[NoSquash] Replace settings tab with button #13762

Merged
merged 2 commits into from Sep 9, 2023

Conversation

rubenwardy
Copy link
Member

@rubenwardy rubenwardy commented Aug 28, 2023

Part of #13476

image
image
image

To do

This PR is Ready for Review

  • Icon

How to test

  • Open main menu
  • Click settings button
  • See settings

@rubenwardy rubenwardy added @ Startup / Config / Util @ Mainmenu Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) labels Aug 28, 2023
@rubenwardy rubenwardy added this to the 5.8.0 milestone Aug 28, 2023
@Zughy
Copy link
Member

Zughy commented Aug 28, 2023

Graphically speaking is pretty bad, the icon was more subtle. Icon + text, if any

@rubenwardy
Copy link
Member Author

This is still a draft PR - I was going to ask you for the icon again

@rubenwardy rubenwardy force-pushed the remove_settings_tab branch 2 times, most recently from 983a677 to 11a526e Compare August 28, 2023 21:39
@rubenwardy rubenwardy added WIP The PR is still being worked on by its author and not ready yet. and removed Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) labels Aug 28, 2023
@dax-er
Copy link

dax-er commented Aug 29, 2023

I think an icon would be much better than a button, but the problem is that the art style wouldn't match the rest of the main menu. imo the tabs and other buttons should also be changed to have more of a pixel art style. possibly for a different PR. Also I think an icon is enough for the settings, since less text means less translations overflowing to the side.

@rubenwardy rubenwardy changed the title Replace settings tab with button [NoSquash] Replace settings tab with button Aug 29, 2023
@rubenwardy rubenwardy removed the WIP The PR is still being worked on by its author and not ready yet. label Aug 29, 2023
@rubenwardy rubenwardy marked this pull request as ready for review August 29, 2023 22:17
@rubenwardy
Copy link
Member Author

Updated with icon provided by Zughy

Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the bug on Windows, the PR looks good to me.

builtin/fstk/tabview.lua Outdated Show resolved Hide resolved
builtin/fstk/tabview.lua Outdated Show resolved Hide resolved
builtin/fstk/tabview.lua Outdated Show resolved Hide resolved
builtin/fstk/tabview.lua Outdated Show resolved Hide resolved
@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Aug 30, 2023
@rubenwardy rubenwardy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Sep 6, 2023
@rubenwardy rubenwardy force-pushed the remove_settings_tab branch 2 times, most recently from a25f14b to 0ab1b9d Compare September 6, 2023 21:34
Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except a small change to the documentation, this PR looks good to me.

doc/fst_api.txt Outdated Show resolved Hide resolved
@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Sep 7, 2023
@srifqi srifqi added >= Two approvals ✅ ✅ and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Sep 8, 2023
@grorp grorp merged commit 48ab183 into minetest:master Sep 9, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants