Create new feature "equipment".#143
Conversation
Create a new feature "equipment". Adds a page to select user equipment. Adds multiple components to select and manage characters equipments. Adds assets for the equipments.
adapap
left a comment
adapap
left a comment
There was a problem hiding this comment.
Looks great! Thank you for adding in all of the data for this and wiring it up to characters. I left a lot of Vue-related suggestions. My most important comment is to change the way that the assets are handled, because this breaks other places that use existing equipment images.
| <img | ||
| class="equipment-img border border-secondary me-3 mt-3" | ||
| :src="Assets.EquipmentImage(curSlot.name, selectedTypeRef)" | ||
| data-bs-toggle="modal" | ||
| data-bs-target="#equipment-selector" |
There was a problem hiding this comment.
This will be changed in the redesign to use Quasar's q-popup-proxy. Not blocking, just letting you know so that you don't spend too much time on UI before 2.0 launches, which will make these things a lot easier!
| <label for="equipment-luck">Luck</label> | ||
| <input | ||
| id="equipment-luck" | ||
| class="equipment-input" | ||
| type="number" | ||
| :min="0" | ||
| v-model.number="curSlot.luck" | ||
| /> | ||
| <label for="equipment-reach">Reach</label> | ||
| <input | ||
| id="equipment-reach" | ||
| class="equipment-input" | ||
| type="number" | ||
| :min="0" | ||
| v-model.number="curSlot.reach" | ||
| /> |
There was a problem hiding this comment.
Would love to replace all of these with a v-for loop. Add a variable in setup to keep track of these different fields, or just iterate on curSlot.
| watch(selectedIdRef, () => { | ||
|
|
||
| if (curCharacter.value && (!curSlot!.value || Object.keys(curSlot!.value).length === 0)) { // Tests if object exists and is not {} | ||
| curSlot!.value = EquipmentUtilities.getEmptyEquipment(); | ||
| } | ||
| }) |
There was a problem hiding this comment.
It sounds like curSlot would be better suited as a Vue computed property. This automatically updates the value if any of these dependencies change. In general, prefer computed over watch unless you need explicit side-effects.
| // FIXME: This is an ugly but working way to deep-clone an object... | ||
| this.curSlot = JSON.parse(JSON.stringify(newEquipment)); | ||
| } |
There was a problem hiding this comment.
Fine for now. Redesign will include lodash which has lodash.cloneDeep. Still seems like you want a computed property though, you can configure get and set to make getters/setters.
| @@ -42,6 +43,7 @@ export class Character { | |||
| public statues: Record<string, number>; | |||
| public constellations: Record<string, boolean>; | |||
| public starSigns: Record<string, boolean>; | |||
| public equipment: Record<string, Equipment>; | |||
There was a problem hiding this comment.
Can we make the key here more explicit? I'd expect equipment to have a static key type such as "helmet" | "chest" | "pants" | "ring1" | ....
There was a problem hiding this comment.
Looks like EquipmentType could be a starting point?
| <div class="flex-row"> | ||
| <EquipmentsIcon | ||
| v-bind:equipmentType="EquipmentType.FishingLine" | ||
| v-bind:equipmentId="'fishingline'" | ||
| @clicked="equipmentSelected" | ||
| /> | ||
| <EquipmentsIcon | ||
| v-bind:equipmentType="EquipmentType.FishingWeight" | ||
| v-bind:equipmentId="'fishingweight'" | ||
| @clicked="equipmentSelected" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Might be worth having a prop or something for these non-standard cases? It actually might be better to move these to a separate component/logic since it's not equippable in the same way as other equipment. (can you even see this from the equip menu in-game?)
| <EquipmentsIcon | ||
| v-bind:equipmentType="EquipmentType.Food" | ||
| v-bind:equipmentId="'food1'" | ||
| @clicked="equipmentSelected" | ||
| /> |
There was a problem hiding this comment.
Same thing here. Consider a different component/implementation for food since it can't be upgraded/have different base stats, just bonus multipliers.
| <span class="stat-line">Stats for the current equipment : </span> | ||
| <span class="stat-line">Weapon Power : {{ curEquipmentStats.weaponPower }}</span> | ||
| <span class="stat-line">Speed : {{ curEquipmentStats.speed }}</span> | ||
| <span class="stat-line">Defense : {{ curEquipmentStats.defense }}</span> | ||
| <span class="stat-line">Strength : {{ curEquipmentStats.strength }}</span> | ||
| <span class="stat-line">Agility : {{ curEquipmentStats.agility }}</span> | ||
| <span class="stat-line">Wisdom : {{ curEquipmentStats.wisdom }}</span> | ||
| <span class="stat-line">Luck : {{ curEquipmentStats.luck }}</span> | ||
| <span class="stat-line">Reach : {{ curEquipmentStats.reach }}</span> |
There was a problem hiding this comment.
v-for is a powerful friend. Use it frequently!
|
|
||
| let curEquipmentStats = EquipmentUtilities.getCurrentSlotStats(); | ||
|
|
||
| let currentTab = ref(<number>0); |
There was a problem hiding this comment.
(nit): I think ref<Number>(0) is a better definition, since it binds the type of the ref value.
| }; | ||
| const defaultTab = 'Characters'; | ||
| const defaultTab = 'Equipment'; |
There was a problem hiding this comment.
Navigation is changing in redesign but I don't think this should be the default? This feature will probably get moved to the character page since it's related to each character rather than a standalone feature.
|
Superceded by #204 |
2 participants
Create a new feature "equipment".
Adds a page to select user equipment.
Adds multiple components to select and manage characters equipments.
Adds assets for the equipments.