-
Notifications
You must be signed in to change notification settings - Fork 14
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
Popup Manager #28
Popup Manager #28
Conversation
…d with Basic, Mulligan and Concede Variants
const renderPopup = (type: PopupType, data: PopupData) => { | ||
switch (type) { | ||
case "default": | ||
return <DefaultPopupModal data={data as DefaultPopup} />; |
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.
I think this a fine direction for this as far as giving us the ability to render different kinds of popups. However, I would think less about rendering specific popup and more about popup "forms" For instance, here, the concede and mulligan popups are essentially identical to the default popup. In fact both of these could be created using the default popup.
It may be that you only included these as a demonstration of selecting the popup type, but certainly the default popup is on the right track for what we want. I think it will be helpful for you to get into a game and start looking at some of the prompts we're receiving fromt he back end. I think even just this default prompt would handle a lot of cases if we change it to render the buttons provided by the prompts instead of just a yes no. I would take a look at the BasicPrompt already in the frontend code as a reference for how the default might work.
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.
Oh I see. About mulligan and concede, I saw a possible interest for a variant of design(red button because it's important action, or anything imaginable), but we can definitely make it part of the data object we pass to the openPopup method.
About the different prompts, I know that 80% of the prompts can be handled using the basic variant, it was mainly to show flexibility of coding. For the final versions, I see 4 variants:
- Basic ("yes" "no" method)
- Select card(s)
- Probably select from pile (https://www.figma.com/design/eOPqFG1725zHly0vv6s3IH/Karabast?node-id=5167-13172&t=LamatG1bC7hNBWSQ-1) (optional parameters could make it fit into the Select card(s) one)
- the Pile one (see discard/deck)
|
||
const PopupContext = createContext<PopupContextProps | undefined>(undefined); | ||
|
||
export const PopupProvider: React.FC<{ children: React.ReactNode }> = ({ |
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.
Do we have support for multiple popups here or is that still WIP?
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.
Not yet. But refactoring the code to handle multiple popups will not be that struggling
…ve the SelectFromPile one and put the logic into SelectCards one
… into Balkhrog/Popup-Manager
…e selecting cards
… into Balkhrog/Popup-Manager
… into Balkhrog/Popup-Manager
…ty change when popup is open, allow clicking throught the overlay
… into Balkhrog/Popup-Manager
How to start using the Popup Manager:
If you need to make a variant of the popup (that cannot be handled by the current one and the basic one):