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

"Browse online content" formspec improvement #10756

Merged
merged 10 commits into from
Jan 2, 2021
Merged

Conversation

Zughy
Copy link
Member

@Zughy Zughy commented Dec 27, 2020

Closes #10069

image
image

  1. Descriptions have been moved in tooltips. It now says "Install mod name" and such when hovering on buttons
  2. Irrlicht screws pixel art up when it's scaled with a decimal value, but apparently more details make it less evident
  3. There were tons of table concatenations. I kept some, but in some other cases they made the code incomprehensible, so I got rid of them
  4. Download icon is animated, but my PC is a toaster and I can't record a decent GIF
  5. "View" string has been removed, it was too generic
  6. Yes, main menu needs a complete overhaul, but in the meanwhile this allows me to drop a few decent icons and to make it less ugly
  7. PLEASE don't lose yourself in tiny aesthetic details, this is an easy one. Ask yourself: is this better than before? Yes/no. If the answer is yes and the code is good, approve it. If no, junk it

To do

This PR is Ready for Review

How to test

Open MT, open CDB formspec, click icons

@ghost
Copy link

ghost commented Dec 27, 2020

Cool icons and colors, I love them instead of the labeled buttons.

@rubenwardy rubenwardy added @ Content / PkgMgr Feature ✨ PRs that add or enhance a feature labels Dec 31, 2020
@rubenwardy
Copy link
Member

These icons are a definite improvement

@rubenwardy rubenwardy self-requested a review January 1, 2021 21:16
Copy link
Member

@rubenwardy rubenwardy left a comment

Choose a reason for hiding this comment

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

There were tons of table concatenations. I kept some, but in some other cases they made the code incomprehensible, so I got rid of them

FYI: table concatenation is used because string concat (..) is expensive in Lua, especially with large strings.

Small uses of string concat are probably OK though. If you are concating lots of things, it may be better to use format - but a lot of the stuff here is fairly short I guess

builtin/mainmenu/dlg_contentstore.lua Outdated Show resolved Hide resolved
builtin/mainmenu/dlg_contentstore.lua Outdated Show resolved Hide resolved
builtin/mainmenu/dlg_contentstore.lua Outdated Show resolved Hide resolved
builtin/mainmenu/dlg_contentstore.lua Show resolved Hide resolved
@rubenwardy
Copy link
Member

The internet icon doesn't look very clear to me. There's a lot of lines that all merge together. Could it be simplified a bit somehow?

@rubenwardy
Copy link
Member

You've accidentally included the clear icon in this PR

@rubenwardy rubenwardy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jan 2, 2021
@Zughy
Copy link
Member Author

Zughy commented Jan 2, 2021

New icon, it should be clearer now
imagen

All requests have been addressed. I also used the new code style length in some cases, where 95 has been used as a soft limit (I've reached 100 characters, max)

@rubenwardy
Copy link
Member

Yeah, that's better

@rubenwardy rubenwardy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jan 2, 2021
po/minetest.pot Outdated Show resolved Hide resolved
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

Looks good.

@sfan5 sfan5 merged commit 92aac69 into minetest:master Jan 2, 2021
@Zughy Zughy deleted the cdb_icons branch January 2, 2021 15:30
@Zughy Zughy mentioned this pull request Feb 21, 2021
1 task
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.

ContentDB uninstall button slightly too small for translation
3 participants