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

feat: stats ui + inventory theme #166

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

StonyTV
Copy link

@StonyTV StonyTV commented Nov 17, 2024

This PR includes a redesign of the inventory theme as well as the addition of the characteristics menu.

Inventory appearance:
image

Characteristics menu image:
image

Copy link

vercel bot commented Nov 17, 2024

@StonyTV is attempting to deploy a commit to the aresrpg Team on Vercel.

A member of the Team first needs to authorize it.

@StonyTV StonyTV requested a review from Sceat November 17, 2024 17:54
@StonyTV StonyTV self-assigned this Nov 17, 2024
Copy link
Member

@Sceat Sceat left a comment

Choose a reason for hiding this comment

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

Congrats on your first pull request

selected_character.value.wisdom !== wisdom ||
selected_character.value.agility !== agility
) {
selected_character.value = current_character(state);
Copy link
Member

Choose a reason for hiding this comment

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

rather than using the expensive current_character function
image

You could assign { vitality, strength, .. } to selected_character.value

Comment on lines +147 to +148
position absolute;
z-index 1;
Copy link
Member

Choose a reason for hiding this comment

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

semi-colon are not needed in stylus, does the npm run format pickup on those change?

Comment on lines +5 to +6
// @todo use image_url
img(:src="`https://assets.aresrpg.world/classe/${selected_character.classe}_${selected_character.sex}.jpg`")
Copy link
Member

Choose a reason for hiding this comment

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

todo?


const accept_loading = ref(false);

const stats = ref({
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be a ref ? also vue ref for objects are called reactive but it seems a simple const would work here

const stats = ref({
vitality: {
label: t('APP_ITEM_VITALITY'),
imagePath: new URL('../../assets/statistics/vitality.png', import.meta.url).href
Copy link
Member

Choose a reason for hiding this comment

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

usually we import the image at the top of the vue file to let the bundler generate the url

import vitality_image from '../../assets/statistics/vitality.png'

const stats = { vitality: { label, url: vitality_image } }

//- FluentCheckmark12Regular
//- span Valider
.right
vs-button.cancel(icon color="#E74C3C" v-if="has_pending_allocated_stats()" @click="cancel_pending_allocated_stats")
Copy link
Member

Choose a reason for hiding this comment

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

same thing about using function in rendering, this will slow down the app

}


async function validate_stats() {
Copy link
Member

Choose a reason for hiding this comment

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

validate is misleading as it has a different meaning in French and in English, better to use commit or execute



async function validate_stats() {
accept_loading.value = true;
Copy link
Member

Choose a reason for hiding this comment

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

For each actions triggering a blockchain transaction, let's add a confirm modal. Especially important for ZkLogin which won't need to approve in-wallet. The only case of no modal would be using an item like Bread so that it doesn't impact the gameplay, but for stats it seems important


sdk.add_header(tx)

const { kiosks, finalize } = await sdk.get_user_kiosks({
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const { kiosks, finalize } = await sdk.get_user_kiosks({
const { get_kiosk_cap_ref, finalize } = await sdk.get_user_kiosks({

The new sdk version directly provide that function

Comment on lines +171 to +175
context.dispatch('action/character_update', {
id: selected_character.value.id,
...Object.keys(stats.value).map(stat => ({ [stat]: selected_character.value[stat] + (allocated_stats.value[stat] ?? 0) })),
available_points: selected_character.value.available_points - get_pending_allocated_stats_count(),
})
Copy link
Member

Choose a reason for hiding this comment

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

Let's have a call about that as I implemented a more robust system with the item usage feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants