Skip to content
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

add "reward maker" ext #1194

Draft
wants to merge 13 commits into
base: develop
Choose a base branch
from
Draft

Conversation

CH3RVB
Copy link

@CH3RVB CH3RVB commented Jan 13, 2025

it's basically an ext that allows for easier rewarding for loot like through prompts, also replaces the existing prompt reward system to streamline it

worth noting that this is a very VERY rough draft because i wanted to get feedback and see what the general opinion was-- it's a bit of mix of the old default char rewards PR + my reward maker ext (edited + added reward_key so a model can theoretically have multiple different "reward sets"

things i believe i will have to edit/add:

  • notif for when a character not owned by the prompt's submitter is set as focus and receiving rewards
  • remove migrated prompt rewards from the DB

this was a suggestion i got so i'm definitely feeling like i may have fumbled along the way somewhere, but like i said-- just looking for proper feedback so i can see where to take all of this (do we even want to replace prompt rewards with this or think it's a good idea???????)

@preimpression
Copy link
Contributor

Could the tables included in this to be swapped over to a responsive div format?

@CH3RVB
Copy link
Author

CH3RVB commented Jan 15, 2025

Could the tables included in this to be swapped over to a responsive div format?

i just woke up so i might be short a few braincells ahahaha but i'm not getting what you're meaning by this, could you explain?

@SpeedyD
Copy link
Contributor

SpeedyD commented Jan 15, 2025

Could the tables included in this to be swapped over to a responsive div format?

i just woke up so i might be short a few braincells ahahaha but i'm not getting what you're meaning by this, could you explain?

In your code I saw a block looking like this, in resources/views/widgets/_reward_display.blade.php:

      <table class="table table-sm">
        <thead>
            <tr>
                <th width="70%">Reward</th>
                <th width="30%">Amount</th>
            </tr>
        </thead>

A sort-of equivalent would be this, for example, from /resources/views/admin/items/items.blade.php:

        <div class="mb-4 logs-table">
            <div class="logs-table-header">
                <div class="row">
                    <div class="col-5 col-md-6">
                        <div class="logs-table-cell">Name</div>
                    </div>
                    <div class="col-5 col-md-5">
                        <div class="logs-table-cell">Category</div>
                    </div>
                </div>
            </div>

Basically: Use bootstrap for the rows and columns.

@CH3RVB
Copy link
Author

CH3RVB commented Jan 15, 2025

In your code I saw a block looking like this, in resources/views/widgets/_reward_display.blade.php:

      <table class="table table-sm">
        <thead>
            <tr>
                <th width="70%">Reward</th>
                <th width="30%">Amount</th>
            </tr>
        </thead>

A sort-of equivalent would be this, for example, from /resources/views/admin/items/items.blade.php:

        <div class="mb-4 logs-table">
            <div class="logs-table-header">
                <div class="row">
                    <div class="col-5 col-md-6">
                        <div class="logs-table-cell">Name</div>
                    </div>
                    <div class="col-5 col-md-5">
                        <div class="logs-table-cell">Category</div>
                    </div>
                </div>
            </div>

Basically: Use bootstrap for the rows and columns.

OH RIGHT yeah i can do that for sure! 🫡

Copy link
Contributor

@ScuffedNewt ScuffedNewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small review for now will look in more detail later


foreach ($rewards as $reward) {
switch ($reward->object_type) {
case 'Questline':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely don't include questlines in this in the final PR

case 'Questline':
$objmodel = 'App\Models\Questline\Questline';
break;
case 'Prompt':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also dont need to do this at all and can instead use an object relation on the ObjectReward model

* Get the object.
*/
public function object() {
return $this->morphTo(__FUNCTION__, 'object_type', 'object_id');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats the purpose of the __FUNCTION__ ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of having an object reward model why not just have the stored assets array?

public function rewards() {
return $this->hasMany(PromptReward::class, 'prompt_id');
public function objectRewards() {
return $this->hasMany('App\Models\ObjectReward', 'object_id')->where([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally this would use the ::class

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is pulled from claymores, why not just pull the migration filename so that the migration errors dont happen?

@itinerare itinerare added enhancement New feature or request needs review Pull requests that are pending community review labels Jan 24, 2025
@CH3RVB CH3RVB marked this pull request as draft January 27, 2025 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs review Pull requests that are pending community review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants