Merged
Conversation
- Introduced SVG icons for hybrid mana symbols: black, blue, green, red, and white. - Added corresponding icons for colorless hybrid combinations with each color. - Ensured proper scaling and transformation for consistent rendering in the application.
- Introduced SVG icons for all hybrid phyrexian mana symbols, including combinations of black, blue, green, red, and white. - Ensured proper scaling and transformation for consistent rendering across the application.
- Removed outdated note about hybrid mana being "coming soon." - Added detailed descriptions and icons for all hybrid and phyrexian mana symbols. - Organized mana symbols into clear sections for better readability.
- Organized SVG icons for Phyrexian mana symbols: black, blue, green, red, white, and colorless.
- None of these are needed in documentation - This cleanup helps streamline the icon library and reduces unnecessary file clutter.
- Added IconFilenameService to map MTG mana costs and symbols to icon types. - Included methods for basic colors, special symbols, numeric symbols, phyrexian symbols, and hybrid symbols. - Created comprehensive tests to ensure correct mapping and functionality.
- Updated IconService to introduce symbol_svg method for rendering various mana symbols, including numeric and phyrexian symbols.
- Refactored ManaCost to store elements as formatted symbol strings (e.g., '{2}', '{R}') instead of symbols.
- Improved tests to validate new symbol handling and ensure correct SVG rendering for all mana types.
- Removed deprecated methods and streamlined icon rendering logic.
- Adjusted paths for Phyrexian mana icons in the README to reflect the new directory structure. - Ensured consistency in icon references for all Phyrexian mana types, enhancing clarity and accuracy.
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive support for hybrid mana symbols to the MTG card maker, expanding beyond basic mana and Phyrexian symbols to include two-color hybrids, "twobrid" symbols (2/color), and Phyrexian hybrid combinations. The implementation introduces a new IconFilenameService to handle the complex symbol-to-filename mapping and refactors the existing services to use a unified symbol_svg method.
- Adds support for hybrid mana symbols ({W/B}, {2/W}, {W/B/P}, etc.)
- Introduces IconFilenameService for centralized symbol mapping
- Refactors IconService and related components to use unified symbol_svg method
- Updates all test expectations to use new symbol format (e.g., '{R}' instead of :red)
Reviewed Changes
Copilot reviewed 10 out of 126 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/mtg_card_maker/icon_filename_service.rb | New service that maps symbol strings to icon types, supporting basic, hybrid, phyrexian, and numeric symbols |
| lib/mtg_card_maker/icon_service.rb | Refactored to use symbol_svg method with IconFilenameService for mapping, added subdirectory support for hybrid/phyrexian icons |
| lib/mtg_card_maker/symbol_replacement_service.rb | Simplified to use unified symbol_svg method, removed hardcoded symbol mapping |
| lib/mtg_card_maker/mana_cost.rb | Updated to store symbol strings instead of icon types, uses unified symbol_svg method |
| spec/ files | Updated test expectations to use new symbol string format and unified API methods |
| lib/mtg_card_maker.rb | Added require for new IconFilenameService |
| README.md | Updated documentation to reflect new hybrid mana support and corrected icon paths |
Comments suppressed due to low confidence (3)
lib/mtg_card_maker/icon_filename_service.rb:107
- The method
extract_number_from_symbolreturns 'X' as a string when no match is found, but the calling code expects an Integer. This will cause thenumber.is_a?(Integer)check to fail and return nil unexpectedly.
end.to_h
spec/mtg_card_maker/mana_cost_spec.rb:168
- The test description 'uses icons for all mana symbols' doesn't match the expectation. The test is checking for SVG count but the description suggests it's testing icon usage for all symbols, which should verify that circles are no longer used for colorless mana.
expect(svg.scan('<svg').size).to eq(3) # All symbols now use icons
spec/mtg_card_maker/mana_cost_spec.rb:171
- This test description appears to contradict the changes made. If all symbols now use icons (as stated in line 165), then this test about using circles for colorless mana seems inconsistent with the new behavior.
it 'uses circles for colorless mana', :aggregate_failures do
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces significant updates to the the internal handling of mana symbols in the codebase. The changes enhance support for hybrid and Phyrexian mana symbols, improve the modularity of the code, and ensure better maintainability by introducing a dedicated service for icon mapping.
Documentation Updates:
{R/G}and{G/W/P}, along with corresponding icons (README.md). [1] [2]README.md). [1] [2]Code Enhancements:
New Services and Modularization:
IconFilenameServiceto centralize the mapping of mana symbols (e.g.,{W},{2},{W/B}) to icon types, enabling easier maintenance and extensibility (lib/mtg_card_maker/icon_filename_service.rb).IconServiceto useIconFilenameServicefor symbol-to-icon mapping and added support for rendering hybrid and numeric mana symbols (lib/mtg_card_maker/icon_service.rb). [1] [2] [3]Refactoring of Mana Cost Parsing:
ManaCostwith dynamic parsing of symbol strings, allowing for greater flexibility and alignment with the newIconFilenameService(lib/mtg_card_maker/mana_cost.rb). [1] [2] [3]File Structure Adjustments:
IconFilenameServicein relevant modules (lib/mtg_card_maker.rb,lib/mtg_card_maker/icon_service.rb). [1] [2]These changes collectively improve the user experience by expanding the supported mana symbols and enhance the codebase's maintainability through better modularization and dynamic handling of mana symbols.