feat: add gong sound + cta for settling realm and joining battle #2149
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request includes several modifications across multiple components and hooks in the codebase. Key changes involve the simplification of the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
client/src/ui/elements/TwitterShareButton.tsx (2)
11-11: Consider adding a default prop valueWhile the implementation works, providing a default value for
callToActionTextcould improve developer experience and make the component more maintainable.-const TwitterShareButton: React.FC<Props> = ({ text, className, callToActionText }) => { +const TwitterShareButton: React.FC<Props> = ({ + text, + className, + callToActionText = 'Share to Twitter' +}) => {
20-20: Extract default text to a constantThe default text "Share to Twitter" is hardcoded and could be duplicated if implementing default props. Consider extracting it to a constant for better maintainability.
+const DEFAULT_SHARE_TEXT = 'Share to Twitter'; + const TwitterShareButton: React.FC<Props> = ({ text, className, - callToActionText + callToActionText = DEFAULT_SHARE_TEXT }) => { // ... - {callToActionText ? callToActionText : `Share to Twitter`} + {callToActionText}client/src/hooks/helpers/use-settle-realm.tsx (3)
32-36: Simplify array handling in create_multiple_realms call.While the calldata construction has been simplified, we can further improve it by removing the redundant spread operator.
- const calldata = [Number(id)]; - - await create_multiple_realms({ - signer: account, - realm_ids: [...calldata], - }); + await create_multiple_realms({ + signer: account, + realm_ids: [Number(id)], + });
Line range hint
40-46: Improve error handling and type safety for settleRealm return value.The function returns
idonly in the success case but implicitly returnsundefinedin the error case. This could lead to type safety issues in consuming components.- return id; + return { success: true as const, id }; } catch (error) { setErrorMessage("Realm already settled. Please try a different Realm."); console.error("Error during minting:", error); + return { success: false as const, error: String(error) }; } finally { setIsLoading(false); }Then update the return type:
type SettleRealmResult = | { success: true; id: number } | { success: false; error: string };
Line range hint
7-11: Consider optimizing state management.The hook maintains several pieces of state (
isLoading,selectedOrder,tokenId,errorMessage). SinceselectedOrderandtokenIdmight no longer be needed after removing order-based selection, consider simplifying the state management.- const [selectedOrder, setSelectedOrder] = useState(1); - const [tokenId, setTokenId] = useState<number>(0);Then update the return value:
- return { settleRealm, isLoading, selectedOrder, setSelectedOrder, tokenId, setTokenId, errorMessage }; + return { settleRealm, isLoading, errorMessage };client/src/ui/modules/onboarding/Steps.tsx (2)
326-330: Add error handling for realm name retrievalThe
getRealmNameByIdfunction could potentially return undefined if the realm ID is invalid. Consider adding a fallback.- const realmName = getRealmNameById(settledRealmId); + const realmName = getRealmNameById(settledRealmId) ?? 'My realm'; return ( <TwitterShareButton text={`I've joined the Eternum battle for glory.\nWars will be fought, tears will be shed.\n${realmName} has been settled. ⚔️`} />
337-343: Consider improving mobile responsivenessThe current grid layout might not provide the best experience on mobile devices. Consider adding responsive classes.
- <div className="grid grid-cols-3 gap-4 justify-center items-center"> - <div className="col-start-2 flex justify-center"> + <div className="grid grid-cols-1 md:grid-cols-3 gap-4 justify-center items-center"> + <div className="col-start-1 md:col-start-2 flex justify-center"> <Button size="md" className="mt-4" variant="primary" onClick={onNext}> Continue <ArrowRight className="w-2 fill-current ml-3" /> </Button> </div> - <div className="col-start-3 flex justify-start"> + <div className="col-start-1 md:col-start-3 flex justify-center md:justify-start"> {settledComponent && settledComponent} </div>client/src/ui/modules/navigation/TopLeftNavigation.tsx (1)
345-345: Consider debouncing the gong sound effectThe current implementation might trigger the sound effect too frequently if progress updates rapidly. Consider adding debounce to prevent sound overlap and improve user experience.
Here's a suggested implementation using debounce:
+ import { debounce } from 'lodash'; const TickProgress = () => { const { play } = useUiSounds(soundSelector.gong); + const debouncedPlay = useMemo( + () => debounce(play, 1000, { leading: true, trailing: false }), + [play] + ); useEffect(() => { if (lastProgressRef.current > progress) { - play(); + debouncedPlay(); } lastProgressRef.current = progress; - }, [progress]); + }, [progress, debouncedPlay]);Also applies to: 359-365
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
client/public/sound/events/gong.mp3is excluded by!**/*.mp3
📒 Files selected for processing (7)
client/src/hooks/helpers/use-settle-realm.tsx(2 hunks)client/src/hooks/useUISound.tsx(1 hunks)client/src/ui/components/cityview/realm/SettleRealmComponent.tsx(2 hunks)client/src/ui/elements/TwitterShareButton.tsx(2 hunks)client/src/ui/modules/military/battle-view/BattleSideView.tsx(2 hunks)client/src/ui/modules/navigation/TopLeftNavigation.tsx(5 hunks)client/src/ui/modules/onboarding/Steps.tsx(2 hunks)
🔇 Additional comments (13)
client/src/ui/elements/TwitterShareButton.tsx (2)
8-8: LGTM! Props interface is well-defined
The new optional prop is properly typed and follows TypeScript best practices.
12-12: Verify text sanitization
While encodeURIComponent provides URL encoding, ensure that the text prop is properly sanitized before being used in the Twitter URL to prevent potential XSS attacks.
client/src/hooks/helpers/use-settle-realm.tsx (1)
13-13: Verify removal of order-based realm selection.
The removal of getNextRealmIdForOrder and getRealmIdForOrderAfter functions suggests a shift to random-only realm selection. However, selectedOrder state is still maintained but might be unused.
client/src/ui/components/cityview/realm/SettleRealmComponent.tsx (1)
11-11: LGTM! Verify parent component updates.
The new prop follows TypeScript best practices with clear typing.
Let's verify that all parent components have been updated to handle this breaking change:
✅ Verification successful
All parent components correctly handle the breaking change ✅
The verification shows that SettleRealmComponent is only used in Steps.tsx and it correctly passes the required setSettledRealmId prop, matching the new component signature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components rendering SettleRealmComponent
rg -l "SettleRealmComponent" | xargs rg -A 5 "<SettleRealmComponent"
Length of output: 699
client/src/ui/modules/military/battle-view/BattleSideView.tsx (2)
9-9: LGTM!
The TwitterShareButton import follows the project's import organization pattern.
115-120: Verify component visibility in scrollable container
The TwitterShareButton is placed inside a div with max-h-24 overflow-y-auto. Ensure the button remains visible and accessible when the army list is long.
✅ Verification successful
Based on the search results, I can now generate the final response:
TwitterShareButton placement is safe in scrollable container
The TwitterShareButton is placed at the end of the army list, after all army entries, which is a good practice. The max-h-24 (96px) height constraint provides sufficient space for both the army list and the button, and the button will remain fixed at the bottom of the list. This is consistent with other scroll container patterns in the codebase where interactive elements are properly positioned.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of TwitterShareButton to see common placement patterns
ast-grep --pattern 'TwitterShareButton'
# Look for scroll container patterns in the codebase
rg 'overflow-y-auto' -A 2 -B 2
Length of output: 13068
client/src/hooks/useUISound.tsx (1)
73-73: Verify the gong sound file existence and integration.
The addition of the gong sound follows the established pattern for event sounds. However, let's ensure proper integration:
Consider adding a comment above the soundSelector object to document the purpose and usage context of different sound categories (ui, buildings, resources, events).
✅ Verification successful
Sound file exists and is properly integrated
The verification shows that:
- The gong sound file exists at the correct location:
client/public/sound/events/gong.mp3 - It follows the established directory structure for event sounds
- It's being used in
TopLeftNavigation.tsxwith proper sound hook integration (useUiSounds) - The implementation uses the standard pattern for sound playback in the application
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the gong sound file existence and its usage in the codebase
# Check if the sound file exists in the public directory
fd "gong.mp3" public/sound/events
# Find components using the new gong sound
rg -l "soundSelector.gong" --type tsx
Length of output: 206
Script:
#!/bin/bash
# Let's try different approaches to verify the sound file and its usage
# 1. Find the sound file in any directory
fd "gong.mp3"
# 2. Find all sound files to understand the correct directory structure
fd ".mp3$"
# 3. Search for gong sound usage with ripgrep (without file type restriction)
rg -l "soundSelector.gong"
# 4. Search for any gong-related code
rg -l "gong"
Length of output: 3830
Script:
#!/bin/bash
# Let's check how the gong sound is being used in TopLeftNavigation.tsx
# Check the implementation details
rg -A 5 -B 5 "soundSelector.gong" client/src/ui/modules/navigation/TopLeftNavigation.tsx
# Also check if there are any volume controls being applied
ast-grep --pattern 'useUISound($$$)' client/src/ui/modules/navigation/TopLeftNavigation.tsx
Length of output: 631
client/src/ui/modules/onboarding/Steps.tsx (2)
21-21: LGTM!
The TwitterShareButton import is correctly placed and properly utilized in the code.
319-332: LGTM! Well-structured component with proper state management
The implementation is clean with:
- Proper state management using TypeScript types
- Efficient memoization of the Twitter share component
- Good separation of concerns
client/src/ui/modules/navigation/TopLeftNavigation.tsx (4)
8-8: LGTM: Sound-related imports are properly organized
The new imports for sound functionality are correctly placed with other hook imports.
350-351: LGTM: Progress tracking implementation is solid
The progress tracking using useRef is implemented correctly, with proper initialization and calculation logic.
Also applies to: 354-356
Line range hint 341-411: LGTM: Sound feature is well-integrated
The new sound functionality is cleanly integrated into the TickProgress component without disrupting existing features such as progress display and tooltips.
Line range hint 1-411: Verify the implementation of CTA functionality
While the gong sound is implemented, the CTA for settling realm and joining battle mentioned in the PR title is not visible in this file.
![mentatbot[bot]](https://avatars.githubusercontent.com/in/837875?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR adds nice quality of life improvements with sound feedback and social sharing capabilities. The gong sound for day cycles and Twitter sharing for key moments like settling realms and battles are good additions. The code is generally well structured with good separation of concerns. A few suggestions around error handling and user control over sounds, but overall this is a solid improvement to the user experience.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
|
@edisontim i decided to change the share button to something that blends more with the app style |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (13)
client/src/ui/socials.ts (1)
1-12: Enhance type documentation for consistency and clarityWhile the type definition is well-structured, the documentation could be improved for consistency and clarity.
Consider applying this documentation update:
type TemplateVariables = { + // name of the opposing player/realm enemyName: string; + // number of attacking troops (formatted) attackerTroops: string; + // number of defending troops (formatted) defenderTroops: string; - // game url + // full game URL (e.g., https://eternum.io/realm/123) url: string; + // name of the player's realm realmName: string; // player name addressName: string; // 300 diamonds, 100 donkeys, 2000 gold resources: string; };client/src/ui/modules/military/battle-view/Battle.tsx (2)
63-66: Consider responsive design for button containerThe layout improvements with
space-x-1anditems-centerare good. Consider adding responsive classes for better mobile experience.-<div className="flex justify-center mb-2 space-x-1 items-center"> +<div className="flex justify-center mb-2 space-x-1 md:space-x-2 items-center flex-wrap gap-y-2">
67-74: Enhance type safety for battle sharing propsThe BattleTwitterShareButton integration looks good, but consider creating a dedicated type for the props to improve maintainability and type safety.
type BattleShareProps = { userArmiesInBattle: ArmyInfo[] attackerArmies: ArmyInfo[] defenderArmies: (ArmyInfo | undefined)[] ownArmySide: string battleAdjusted: ComponentValue<ClientComponents["Battle"]["schema"]> | undefined structure: Structure | undefined }client/src/ui/modules/onboarding/Steps.tsx (2)
320-327: Consider memoizing socialsText calculation.The implementation looks good, but the
socialsTextcalculation could be optimized usinguseMemosince it only needs to be recalculated whenrealmNamechanges.Consider this optimization:
- const socialsText = formatSocialText(twitterTemplates.settle, { - realmName: realmName || "", - url: window.location.origin, - }); + const socialsText = useMemo(() => + formatSocialText(twitterTemplates.settle, { + realmName: realmName || "", + url: window.location.origin, + }), + [realmName] + );
323-326: Consider making the URL configurable.The URL is currently hardcoded to
window.location.origin. Consider making it configurable through environment variables for better flexibility across different environments.Consider using an environment variable:
- url: window.location.origin, + url: env.VITE_PUBLIC_APP_URL || window.location.origin,client/src/ui/elements/TwitterShareButton.tsx (4)
20-20: Consider using CSS hover states instead of managing hover stateManaging the hover effect via the
hoverstate variable increases complexity. You could achieve the same effect using CSS hover pseudo-classes or Tailwind CSS's hover utilities, which are more performant and simplify the code.Apply this refactor to remove the
hoverstate:- const [hover, setHover] = React.useState(false); ... - <Button - ... - onPointerEnter={() => setHover(true)} - onPointerLeave={() => setHover(false)} - > + <Button + ... + > <div className={`flex items-center justify-center mr-2 ${iconSizeClasses[buttonSize]}`}> - <TwitterIcon className={`${hover ? "text-brown" : "text-gold"} transition-all duration-300`} /> + <TwitterIcon className="text-gold hover:text-brown transition-all duration-300" /> </div>
24-28: Handle default icon size and unexpectedbuttonSizevaluesCurrently, the
iconSizeClassesobject only includes mappings for"xs"and"md". IfbuttonSizereceives an unexpected value, it may result inundefined. Consider providing a default size or validating thebuttonSizeprop.You can modify the className assignment to include a default size:
<div className={`flex items-center justify-center mr-2 ${iconSizeClasses[buttonSize] || "h-4 w-4"}`}>Alternatively, ensure that
buttonSizeonly accepts the specified values.
39-39: Consider clarity in call-to-action textThe default
callToActionTextis "Share on X", which might be unclear to users familiar with "Twitter". Since the icon is the Twitter logo, consider whether using "Share on Twitter" might provide better clarity.If desired, update the default text:
- <span>{callToActionText ? callToActionText : "Share on X"}</span> + <span>{callToActionText ? callToActionText : "Share on Twitter"}</span>
39-39: Improve accessibility with aria-labelsAdding
aria-labelortitleattributes to theTwitterIconcan improve accessibility for users relying on screen readers.Example:
<TwitterIcon className={`${hover ? "text-brown" : "text-gold"} transition-all duration-300`} aria-label="Twitter icon" />client/src/ui/modules/military/battle-view/battle-twitter-share-button.tsx (4)
60-63: IncludegetAddressNameFromEntityinuseMemodependencies.The
enemyNamecomputation usesgetAddressNameFromEntity, which might change if the entities data updates. To ensure consistency, include it in the dependencies array.Update the
useMemodependencies:}, [defenderArmies, attackerArmies, ownArmySide + , getAddressNameFromEntity ]);
65-74: Add missing dependencies touseMemohooks.The
totalAttackerTroopsandtotalDefenderTroopscomputations depend on properties ofbattleAdjusted. If these properties change, the memoized values should update accordingly.Include the specific
battleAdjustedproperties in the dependencies:[attackerArmies, battleAdjusted + ?.attack_army_health.current],[defenderArmies, battleAdjusted + ?.defence_army_health.current],
77-80: Clarify conditional logic for choosing Twitter templates.The nested ternary operators in
generateTwitterTextcan be hard to read and maintain.Consider simplifying the conditional logic for better readability:
const text = structure ? structure.isMine ? twitterTemplates.underSiege : twitterTemplates.attacking : twitterTemplates.battling;Optionally, refactor using a function or clearer conditional statements.
89-89: OptimizeuseMemodependencies fortwitterText.The
generateTwitterTextfunction depends on more than the specified dependencies. Ensure all variables used within the function are included.Verify that
structureand any other variables referenced ingenerateTwitterTextare appropriately included in the dependencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
client/src/assets/icons/twitter.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
client/src/ui/components/military/PillageHistory.tsx(2 hunks)client/src/ui/elements/TwitterShareButton.tsx(1 hunks)client/src/ui/modules/military/battle-view/Battle.tsx(2 hunks)client/src/ui/modules/military/battle-view/battle-twitter-share-button.tsx(1 hunks)client/src/ui/modules/onboarding/Steps.tsx(2 hunks)client/src/ui/socials.ts(1 hunks)
🔇 Additional comments (14)
client/src/ui/socials.ts (1)
21-27: 🛠️ Refactor suggestion
Consider Twitter's constraints and template consistency
The templates need to account for Twitter's character limits and maintain consistent styling.
Let's verify the template lengths with maximum-length values:
Consider these improvements:
- Extract the Twitter handle to a constant for maintainability
- Add character limit validation in
formatSocialText - Standardize emoji usage across templates
- Consider shorter alternatives for long templates
+const TWITTER_HANDLE = '@RealmsEternum';
+const MAX_TWEET_LENGTH = 280;
+
export const twitterTemplates = {
battling: `⚔️ My army is battling another army in Eternum! ⚔️\n\nI am battling against {enemyName} with {attackerTroops} troops on my side against {defenderTroops} troops on their side.\n\nJoin the battle at {url} 🛡️`,
underSiege: `🏰 My realm is under siege in Eternum! 🏰\n\n{enemyName} is attacking with {attackerTroops} troops ⚔️ while I'm defending with {defenderTroops} troops 🛡️.\n\nHelp me keep my realm at {url} 👑`,
attacking: `🔥 I'm attacking {enemyName}'s realm in Eternum! 🔥\n\nI have {attackerTroops} troops 🗡️ against their {defenderTroops} troops.\n\nHelp me conquer their realm at {url} 🏰`,
settle: `I've joined the Eternum battle for glory.\nWars will be fought, tears will be shed.\n\n{realmName} has been settled. ⚔️\n\nSettle your realm at {url} and compete for 100K $LORDS`,
- pillage: `I, Ser {addressName}, have just raided {enemyName} from {resources} in @RealmsEternum.💰💰💰\n\nJoin the battle for victory at {url}`,
+ pillage: `I, Ser {addressName}, have just raided {enemyName} from {resources} in ${TWITTER_HANDLE} 💰\n\nJoin the battle at {url}`,
};client/src/ui/modules/military/battle-view/Battle.tsx (1)
18-18: LGTM: Clean import addition
The import statement follows React conventions and maintains good code organization.
client/src/ui/components/military/PillageHistory.tsx (5)
6-8: LGTM! Import changes align with social sharing feature addition.
The new imports support the Twitter sharing functionality and resource formatting improvements.
16-20: LGTM! Proper account context usage.
The account context is correctly destructured from useDojo for player identification.
27-30: LGTM! Well-implemented player identification logic.
The useMemo hook efficiently caches the player check result with proper dependencies.
55-55: LGTM! Clean Twitter share button integration.
The button is properly conditionally rendered and sized appropriately.
56-58: LGTM! Clear player identification in UI.
The conditional rendering and styling effectively distinguish between the current player and others.
client/src/ui/modules/onboarding/Steps.tsx (2)
21-23: LGTM: Clean import organization for social sharing feature.
The new imports are well-organized and properly scoped for the social sharing functionality.
331-335: LGTM: Clean layout implementation.
The button layout is well-structured with proper spacing and conditional rendering of the TwitterShareButton.
client/src/ui/elements/TwitterShareButton.tsx (3)
8-10: Enhance component flexibility with new optional props
Great job adding callToActionText, buttonSize, and variant props to make the TwitterShareButton more configurable and reusable in different contexts.
13-19: Set default prop values in the component
Setting default values for buttonSize and variant directly in the destructuring assignment ensures that the component behaves predictably even if these props are not provided.
31-37: Verify that the Button component supports all passed props
Ensure that the Button component accepts and correctly handles the size, variant, onPointerEnter, and onPointerLeave props. If these are not supported, they may have no effect or cause unexpected behavior.
Run the following script to check the Button component's prop types:
✅ Verification successful
Let me gather more information about the Button component's props.
All props are properly supported by the Button component
The Button component fully supports all the props being passed:
size: Supported with values "xs" | "md"variant: Supported with values "primary" | "secondary" | "success" | "red" | "danger" | "default" | "outline" | "opaque"onPointerEnterandonPointerLeave: These are automatically supported as the component extends React.ComponentPropsWithRef<"button">, which includes all native button props
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the Button component supports the specified props.
# Search for the Button component's prop definitions and check for the props.
fd 'Button.tsx' --exec cat {} \; | rg -A 10 'interface Props' | rg 'size|variant|onPointerEnter|onPointerLeave'
Length of output: 162
Script:
#!/bin/bash
# Search for the complete Button component interface and implementation
fd 'Button.tsx' --exec cat {} \;
Length of output: 15247
client/src/ui/modules/military/battle-view/battle-twitter-share-button.tsx (2)
35-37: Ensure troops counts are accurately calculated.
When summing up the troops counts, ensure that all troop types are included and that the properties exist. The use of army?.troops?.<troop_type> prevents potential undefined errors if troops is not defined.
46-58: Consistent handling of adjusted health in calculateTotalTroops.
In calculateTotalTroops, when adjustedHealth is provided, it's returned directly. When it's not, the total troops are calculated from the armies. Verify that this logic aligns with the intended behavior and that adjustedHealth accurately represents the total health.
Please confirm that adjustedHealth corresponds to the entire army's total health and that bypassing the individual troop counts is appropriate in this context.
client/src/ui/modules/military/battle-view/battle-twitter-share-button.tsx
Show resolved
Hide resolved
client/src/ui/modules/military/battle-view/battle-twitter-share-button.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
client/src/ui/components/bank/LiquidityTable.tsx (1)
31-35: Consider optimizing the filtering performanceThe current implementation performs multiple array operations on every render. Consider memoizing the filtered results using
useMemoand creating a lookup map for resources to avoid repeatedfindoperations.Here's a suggested optimization:
const resourceMap = useMemo( () => Object.fromEntries(resources.map(r => [r.id, r])), [] ); const filteredResources = useMemo(() => { const searchLower = searchTerm.toLowerCase(); return Object.entries(RESOURCE_TIERS).flatMap(([tier, resourceIds]) => { if (tier === "lords") return []; return resourceIds.filter((resourceId) => { const resource = resourceMap[resourceId]; return resource?.trait?.toLowerCase().includes(searchLower) ?? false; }); }); }, [searchTerm, resourceMap]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
client/src/three/sound/utils.ts(1 hunks)client/src/ui/components/bank/LiquidityTable.tsx(1 hunks)landing/schema.graphql(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- client/src/three/sound/utils.ts
- landing/schema.graphql
|
@edisontim i've tried fixing the linter but for some reason it's bugged |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
client/src/ui/elements/TwitterShareButton.tsx (1)
13-27: Consider moving static objects outside the componentThe
iconSizeClassesobject is static and doesn't depend on props or state. Moving it outside the component would prevent unnecessary recreation on each render.+const iconSizeClasses = { + xs: "h-3 w-3", + md: "h-4 w-4", +}; const TwitterShareButton: React.FC<Props> = ({ text, className, callToActionText, buttonSize = "md", variant = "primary", }) => { const [hover, setHover] = React.useState(false); const tweetUrl = `https://twitter.com/intent/tweet?text=${encodeURIComponent(text)}`; - const iconSizeClasses = { - xs: "h-3 w-3", - md: "h-4 w-4", - };client/src/ui/modules/onboarding/Steps.tsx (1)
318-324: Consider optimizing social text generationThe social text is generated even when
realmNameis undefined. Consider moving this logic inside a useEffect or memoizing it to avoid unnecessary computations.- const realmName = settledRealmId ? getRealmNameById(settledRealmId) : undefined; - const socialsText = formatSocialText(twitterTemplates.settle, { - realmName: realmName || "", - url: window.location.origin, - }); + const realmName = useMemo(() => + settledRealmId ? getRealmNameById(settledRealmId) : undefined, + [settledRealmId] + ); + + const socialsText = useMemo(() => + realmName ? formatSocialText(twitterTemplates.settle, { + realmName, + url: window.location.origin, + }) : undefined, + [realmName] + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
client/src/ui/elements/TwitterShareButton.tsx(1 hunks)client/src/ui/modules/onboarding/Steps.tsx(2 hunks)
🔇 Additional comments (4)
client/src/ui/elements/TwitterShareButton.tsx (3)
8-10: LGTM! Well-structured type definitions
The new optional props are well-typed using TypeScript's union types, maintaining backward compatibility while adding flexibility for button customization.
31-41: LGTM! Well-implemented button with proper security and accessibility
The button implementation:
- Correctly uses the new customization props
- Implements smooth hover animations
- Uses proper security attributes for external links
- Maintains good accessibility with clear text labels
Line range hint 1-46: Verify component usage across the codebase
Since this component's interface has been significantly enhanced, let's ensure all existing usages are compatible with these changes.
✅ Verification successful
Let me gather more information about the actual usage of the component to verify the props being passed.
Based on the analysis of the component usage across the codebase, I can now provide the final response:
All usages of TwitterShareButton are compatible with the interface changes
The component is used in three locations, and all usages are passing valid props that match the updated interface:
battle-twitter-share-button.tsx:
<TwitterShareButton
className="h-10"
variant="opaque"
callToActionText="Call to Arms!"
text={twitterText}
buttonSize="md"
/>Steps.tsx:
<TwitterShareButton
buttonSize="md"
variant="default"
className="mt-4 col-start-3 justify-self-start hover:text-gold"
text={socialsText}
/>PillageHistory.tsx:
<TwitterShareButton text={twitterText} buttonSize={"xs"} />All instances use the proper prop types and provide valid values for the optional props. The changes to the component interface have been properly integrated across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of TwitterShareButton to verify proper integration
# Search for TwitterShareButton imports and usage
rg -A 5 "TwitterShareButton" --type tsx --type jsx
# Search for potential test files
fd -e test.tsx -e test.jsx -e spec.tsx -e spec.jsx | xargs rg "TwitterShareButton"
Length of output: 1355
Script:
#!/bin/bash
# Get the implementation details of files using TwitterShareButton
cat client/src/ui/modules/military/battle-view/battle-twitter-share-button.tsx
cat client/src/ui/modules/onboarding/Steps.tsx
cat client/src/ui/components/military/PillageHistory.tsx
Length of output: 26998
client/src/ui/modules/onboarding/Steps.tsx (1)
21-23: LGTM: Clean import organization
The new imports are well-organized and properly scoped for the social sharing functionality.
| <div className="grid grid-cols-3 w-full justify-center items-center"> | ||
| <Button size="md" className="mt-4 col-start-2 justify-self-center" variant="primary" onClick={onNext}> | ||
| Continue <ArrowRight className="w-2 fill-current ml-3" /> | ||
| </Button> | ||
| {true && ( | ||
| <TwitterShareButton | ||
| buttonSize="md" | ||
| variant="default" | ||
| className="mt-4 col-start-3 justify-self-start hover:text-gold" | ||
| text={socialsText} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve layout structure and conditional rendering
Several improvements can be made to the layout and rendering logic:
- The hardcoded
truecondition makes the conditional rendering redundant - The grid layout using magic numbers might not be responsive
- The button placement could be more maintainable using flex layout
- <div className="grid grid-cols-3 w-full justify-center items-center">
- <Button size="md" className="mt-4 col-start-2 justify-self-center" variant="primary" onClick={onNext}>
+ <div className="flex w-full justify-center items-center gap-4">
+ <Button size="md" className="mt-4" variant="primary" onClick={onNext}>
Continue <ArrowRight className="w-2 fill-current ml-3" />
</Button>
- {true && (
+ {socialsText && (
<TwitterShareButton
buttonSize="md"
variant="default"
- className="mt-4 col-start-3 justify-self-start hover:text-gold"
+ className="mt-4 hover:text-gold"
text={socialsText}
/>
- )}
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="grid grid-cols-3 w-full justify-center items-center"> | |
| <Button size="md" className="mt-4 col-start-2 justify-self-center" variant="primary" onClick={onNext}> | |
| Continue <ArrowRight className="w-2 fill-current ml-3" /> | |
| </Button> | |
| {true && ( | |
| <TwitterShareButton | |
| buttonSize="md" | |
| variant="default" | |
| className="mt-4 col-start-3 justify-self-start hover:text-gold" | |
| text={socialsText} | |
| /> | |
| )} | |
| <div className="flex w-full justify-center items-center gap-4"> | |
| <Button size="md" className="mt-4" variant="primary" onClick={onNext}> | |
| Continue <ArrowRight className="w-2 fill-current ml-3" /> | |
| </Button> | |
| {socialsText && ( | |
| <TwitterShareButton | |
| buttonSize="md" | |
| variant="default" | |
| className="mt-4 hover:text-gold" | |
| text={socialsText} | |
| /> | |
| )} | |
| </div> |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
client/src/hooks/helpers/useEntities.tsx (1)
148-149: LGTM! Consider extending sorting to otherRealms for consistency.The addition of name-based sorting for player realms improves the user experience by providing a consistent ordering. The implementation correctly uses
localeCompareand maintains proper order of operations (filter then sort).Consider applying the same sorting to
getOtherRealmsfor complete consistency:const getOtherRealms = (filterFn?: (realm: RealmWithPosition) => boolean) => { return useMemo(() => { - return filterFn ? otherRealms.filter(filterFn) : otherRealms; + const realms = filterFn ? otherRealms.filter(filterFn) : otherRealms; + return realms.sort((a, b) => a.name.localeCompare(b.name)); }, [otherRealms, filterFn]); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
client/src/hooks/helpers/useEntities.tsx(1 hunks)client/src/ui/components/cityview/realm/SettleRealmComponent.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/ui/components/cityview/realm/SettleRealmComponent.tsx
Summary by CodeRabbit
Release Notes
New Features
gongsound effect for user interface interactions.TwitterShareButtonwith customizable properties for improved user engagement.BattleTwitterShareButtonfor sharing battle information on Twitter.Improvements
SettleRealmComponentfor better interaction with settled realm IDs.PillageHistoryfor a more dynamic display and sharing of pillage details.TopLeftNavigationcomponent based on progress changes.useEntitieshook for better organization of realm and structure lists.Bug Fixes
Chores
.prettierignoreto exclude theelizafolder from formatting.