-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Robust optimization & fixing of hero-icon-images #7953
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
base: master
Are you sure you want to change the base?
Conversation
The old method for loading hero icon images in main campaign involved using an old spritesheet(which contained the original heroes only) I reoptimized the process by loading hero images through user DB's hero thang ID, which means the code doesn't need updating in the future when more heroes are introduced.
WalkthroughThis pull request updates the way player hero icons are rendered across the application. The previous sprite-based Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant View
participant HeroIconElement
User->>View: Triggers hero icon update
View->>HeroIconElement: Locate element with class ".player-hero"
View->>View: Compute new image source based on hero variable
View->>HeroIconElement: Set the src attribute to the new image path
HeroIconElement-->>User: Render updated hero icon
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/styles/play/campaign-view.sass (1)
654-654
: Removed unnecessary whitespaceThese blank lines with whitespace should be removed to maintain clean formatting in the stylesheet.
- $spriteSheetSize: 30px - - - - .level-indicator + $spriteSheetSize: 30px + + .level-indicator- margin: 0 2px - - - - .campaign-control-button + margin: 0 2px + + .campaign-control-buttonAlso applies to: 662-662
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/assets/images/pages/play/hero-portrait-frame.png
is excluded by!**/*.png
📒 Files selected for processing (6)
app/styles/clans/clan-details.sass
(1 hunks)app/styles/play/campaign-view.sass
(1 hunks)app/templates/clans/clan-details.pug
(3 hunks)app/templates/play/campaign-view.pug
(1 hunks)app/views/clans/ClanDetailsView.js
(1 hunks)app/views/play/CampaignView.js
(1 hunks)
🔇 Additional comments (8)
app/templates/play/campaign-view.pug (1)
397-397
: Good refactoring of hero icon implementation.Converting from a span with background image to a direct img element is a good approach for making the code more maintainable and scalable. This change aligns perfectly with the PR objective of optimizing hero icon images to handle new heroes dynamically.
app/views/clans/ClanDetailsView.js (1)
350-350
: Excellent improvement to hero icon update logic.This change properly transitions from the previous class-based approach to directly setting the image source, making it much more maintainable. The new implementation dynamically loads hero portraits from the database using the hero's ThangType ID, which ensures the system will automatically work with new heroes without code modifications.
app/styles/clans/clan-details.sass (1)
146-151
: Clean CSS implementation for the new hero image approach.The new
.player-hero
class effectively supports the transition from spritesheet-based icons to individual image elements. The styling provides a consistent frame and sizing while allowing hero portraits to be loaded dynamically. This greatly simplifies the CSS by eliminating the need for hero-specific style rules, making the codebase more maintainable and scalable.app/views/play/CampaignView.js (1)
1724-1729
: Improved hero image loading implementationThis change simplifies the hero updating logic by directly setting the image source rather than swapping CSS classes, making the code more straightforward and maintainable. This approach will also better support new heroes as they're added.
- for (const [slug, original] of Object.entries(ThangType.heroes)) { - if (original === hero) { - this.$el.find('.player-hero-icon').removeClass('player-hero-icon-' + ('collected' in ThangType.heroes ? 'bronze' : 'null')) - this.$el.find('.player-hero-icon').addClass('player-hero-icon-' + slug) + for (const original of Object.values(ThangType.heroes)) { + if (original === hero) { + this.$el.find('.player-hero').attr('src', `/file/db/thang.type/${hero}/portrait.png`)app/templates/clans/clan-details.pug (3)
53-53
: Changed span to img element for clan chieftain heroConverting from a span with CSS sprite background to an actual img element improves semantic HTML and accessibility. This change also aligns with the new approach for hero images throughout the application.
144-144
: Changed span to img element for premium dashboard member heroConsistent with the previous change, this improves semantic HTML by using an img element instead of a styled span for member heroes in the premium dashboard view.
241-241
: Changed span to img element for basic dashboard member heroCompleting the conversion from spans to img elements for all hero icons in the clan details view. This ensures consistency across the application.
app/styles/play/campaign-view.sass (1)
640-648
: Updated player hero styling for new image-based approachThe new CSS for
.player-hero
properly defines dimensions, padding, and margins for the hero images. This replaces the old sprite-based approach with a cleaner, more maintainable solution.- .player-name - margin-left: 45px + .player-hero + height: 30px + width: 30px + background-image: url(/images/pages/play/hero-portrait-frame.png) + padding: 6px + background-size: contain + margin-left: 10px + margin-right: 2px - .player-hero-icon - background: url(/images/pages/play/play-spritesheet.png) no-repeat - width: 30px - height: 30px - display: inline-block - margin: 0px 5px 0px 5px
The old method for loading hero icon images in main campaign involved using an old spritesheet(which contained the original heroes only) I reoptimized the process by loading hero images through user DB's hero thang ID, which means the code doesn't need updating in the future when more heroes are introduced.
Before


After

Summary by CodeRabbit
Style
Refactor