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

18NY - fix salvage rendering #9139

Closed
wants to merge 1 commit into from

Conversation

benjaminxscott
Copy link
Contributor

@benjaminxscott benjaminxscott commented Apr 23, 2023

addresses regression in #9136, adds tests

Please minimize the amount of changes to shared lib/engine code, if possible

If you are implementing a new game, please break up the changes into multiple PRs for ease of review.

Before clicking "Create"

  • Branch is derived from the latest master
  • [NA] Add the pins label if this change will break existing games
  • Code passes linter with docker compose exec rack rubocop -a
  • Tests pass cleanly with docker compose exec rack rake

Implementation Notes

  • Explanation of Change

  • Screenshots

  • Any Assumptions / Hacks

@crericha
Copy link
Collaborator

I have a few issues with this PR:

  1. The new fields are not tied into the game's salvaging logic. There should not be two sources of truth.
  2. The button should change the verb based on which train is selected as there may be a mixture of salvageable and non-salvageable trains.
  3. If we are going to have a common concept of salvage, it should be supported in common code and not game specific code.

I personally don't think this reported issue needs to be fixed, but won't oppose a fix if done properly and completely.

@benjaminxscott
Copy link
Contributor Author

@crericha - I don't disagree, will send a slack DM

@benjaminxscott
Copy link
Contributor Author

Will work on generalizing later on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants