-
Notifications
You must be signed in to change notification settings - Fork 20
Add a two-player bargain template #794
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: main
Are you sure you want to change the base?
Conversation
…safety improvements - Problem 1 (Payout): Added bargain payout calculation based on role and agreed price - Problem 2 (UI): Show deal summary inside dialogue panel instead of covering history - Problem 3 (Validation): Added comprehensive valuation range validation [6-12] - Added maximum turns display in action panel - Removed 'as any' casts by adding BargainStageConfigData to StageConfigData union
| { | ||
| collectionName: 'experiments', | ||
| experimentTemplate, | ||
| experimentTemplate: experimentTemplate, |
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.
Since the field and the variable are the same name, you can just put experimentTemplate, (as was originally there) instead of experimentTemplate: experimentTemplate
| "noImplicitAny": true, | ||
| "module": "es6", | ||
| "target": "es6", | ||
| "module": "es2020", |
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.
Can you revert this?
| "version": "0.10.13", | ||
| "resolved": "https://registry.npmjs.org/@firebase/app/-/app-0.10.13.tgz", | ||
| "integrity": "sha512-OZiDAEK/lDB6xy/XzYAyJJkaDqmQ+BCtOEPLqFvxWKUz5JbBmej7IiiRHdtiIOD/twW7O5AxVsfaaGA/V1bNsA==", | ||
| "peer": true, |
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.
Is it possible to revert all package-lock.json changes for now?
|
|
||
| import CopyWebpackPlugin from 'copy-webpack-plugin'; | ||
| import {Configuration} from 'webpack'; | ||
| import type {Configuration} from 'webpack'; |
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.
Can you revert all webpack.config.ts changes?
| kind: StageKind.BARGAIN, | ||
| isGameOver: false, | ||
| currentTurn: null, | ||
| maxTurns: 8, |
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.
Shouldn't maxTurns and other settings here be set based on the bargain stage config (rather than hardcoded)?
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.
These numbers are just placeholder and will be assigned with randomized values when entering the bargain stage
| // Current turn number (1-indexed, max = maxTurns), null if game not started | ||
| currentTurn: number | null; | ||
| // Maximum number of turns for this game | ||
| maxTurns: number; |
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.
Is this different than what's set in the stage config? (Same question for other fields like chatEnabled).
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.
The value is just a placeholder and will be re-assigned in bargain.utils.ts. I set them to Null here to avoid confusion
| // Public ID of participant whose turn it is to make an offer | ||
| currentOfferer: string | null; | ||
| // Buyer's public ID | ||
| buyerId: string | null; |
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.
The fields buyerId and sellerId seem redundant with participantRoles?
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.
You are right. I will remove them since participantRoles already have the information.
| // For each payout item, only add result to list if item is active; | ||
| // if the payout item has a randomSelectionId, | ||
| // only add the item if it was randomly selected for that participant | ||
| console.log('[PAYOUT] Processing payout items', { |
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.
Please don't add console logs like this in your final PR.
| stageConfigMap: Record<string, StageConfig>, | ||
| publicDataMap: Record<string, StagePublicData>, | ||
| profile: ParticipantProfile, // current participant profile | ||
| participantAnswerMap?: Record<string, BaseStageParticipantAnswer>, |
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.
Don't make this optional
|
|
||
| # Packed shared utils package | ||
| *deliberation-lab-utils-*.tgz | ||
| *deliberation-lab-utils-*.tgz |
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.
Please don't add extra blank lines here
Description