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

Restore details of byproducts/container contents to the crafting gui #56632

Merged

Conversation

ZeroInternalReflection
Copy link
Contributor

Summary

Interface "Restore details of byproducts and container contents to the crafting UI"

Purpose of change

Pull request #51496 added display of information on byproducts and details of items in containers to the results pane of the crafting GUI. However, a subsequent PR working on a different part of the crafting UI (#52685) overwrote the line that actually displayed this information. Since most of the required code is still present and issue #56279 suggests that this is still an area of confusion for players, it makes sense to restore this functionality.

Describe the solution

See #51496 for full discussion of implementation.

Describe alternatives you've considered

See #51496 for discussion of alternatives.

Testing

Tested display of the results pane in a variety of window sizes on a computer monitor.
Did not test with any other physical screen size.
Did not test in any language other than English.

Also tested jumping to an item's recipe to ensure that the relevant code removed was not key to #52685.

Additional context

Appearance of items with no container/byproducts:
Shotshell_Belt--Results_In_Single_Item

Appearance of items with byproducts in addition to the primary result:
Long_Pole--Result_with_discrete_byproducts

Appearance of items with results in containers:
Pickled_Fish--Result_In_Container

Appearance of items with byproducts and primary results in containers:
Canned_Fruit--Result_In_Container_With_Byproduct

Appearance of batch crafting:
Canned_Fruit_Batch

This information was originally added to the crafting gui in PR#51496.
However, a subsequent unrelated change removed the lines that actually
displayed it.
@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Apr 6, 2022
@dseguin
Copy link
Member

dseguin commented Apr 6, 2022

Thanks for fixing this! I think the lines were reverted during a rebase or something. Totally my mistake :P

@Maleclypse Maleclypse added the <Bugfix> This is a fix for a bug (or closes open issue) label Apr 6, 2022
@Delatium
Copy link

Delatium commented Apr 6, 2022

Double "Result" line just doesn't look that good

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 6, 2022
@ZeroInternalReflection
Copy link
Contributor Author

Double "Result" line just doesn't look that good

Other formats can be arranged, but that was the only one that was discussed in the previous Issue/PR. I've also been experimenting with this alternative:
Alternative_Format--Canned_Fruit
Which makes it easier to see where each description starts and ends, but I'm unsure it really suits the rest of the UI.

Whatever the formatting ends up looking like, I think it needs to:

  1. Make it clear to the user what the recipe produces
  2. Make it clear when the user can scroll down for more information
  3. Mesh with the rest of the crafting UI

Recipes with multiple byproducts (e.g. heavy cream) only had one
"Byproduct" header.  Adding a header for each iteminfo section allows
the user to more easily find the details they're looking for.

Several strings that needed to be translated were not being flagged
with _(), and several strings that did not need to be translated (e.g.
_("<bold>") were being flagged for translation.
With the new formatting, this function, which was a wrapper for
readability around adding to an std::vector<iteminfo>, was no longer
used or useful.
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 8, 2022
@ZeroInternalReflection
Copy link
Contributor Author

The last three commits lay out the alternative format I mentioned above:
Batch_Canned_Fruit

I also did some testing in German with placeholder translations to see if I was flagging things correctly. Turns out I wasn't. The original PR had bold tags flagged for translation, but not "Byproduct". I think I've resolved that now (Note: the placeholder translations below are not included in this PR, and are probably wrong):
German_Heavy_Cream

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 13, 2022
@dseguin dseguin merged commit 0a2138c into CleverRaven:master Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants