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.

Sign up for GitHub

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

fix: update currency display in setting for change class #15391

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

fix: update currency display in setting for change class #15391

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e3cdab6
fix: add neededCurrencyOnly prop
CuriousMagpie Feb 5, 2025
e69078f
fix: worked on logic for displaying only gems
CuriousMagpie Feb 6, 2025
a98572c
fix: removed gold & hourglasses if neededCurrencyOnly is true
CuriousMagpie Feb 7, 2025
cc70a5c
fix: aligned gem count properly
CuriousMagpie Feb 7, 2025
e36dde7
fix: updates based on some comments (all comments answered)
CuriousMagpie Feb 10, 2025
791f565
fix: working on displaying only required currency in a new array
CuriousMagpie Feb 14, 2025
834d47a
chore: trying to figure out currency logic
CuriousMagpie Feb 18, 2025
9c64166
trying to use .find() now
CuriousMagpie Feb 19, 2025
9cea367
remove unneeded line of code
CuriousMagpie Feb 19, 2025
c0827f1
still working on finding/filtering the currency
CuriousMagpie Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 33 additions & 22 deletions website/client/src/components/shops/balanceInfo.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<div
v-for="currency of currencies"
:key="currency.key"
:needed-currency-only="neededCurrencyOnly"
class="d-flex align-items-center"
>
<div
Expand Down Expand Up @@ -54,6 +55,9 @@ export default {
amountNeeded: {
type: Number,
},
neededCurrencyOnly: {
type: Boolean,
},
},
data () {
return {
Expand All @@ -67,33 +71,40 @@ export default {
computed: {
currencies () {
const currencies = [];
Copy link
Member

Choose a reason for hiding this comment

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

this is so weird, why do we make an array and push stuff into it instead of just defining the array in the const declaration? or one better, defining the possible currencies in the common code... 🤔 You don't necessarily need to refactor it, just .. huh

Copy link
Member Author

Choose a reason for hiding this comment

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

I rewrote it so the array is defined within the const declaration instead using .push()

currencies.push({
type: 'hourglasses',
icon: this.icons.hourglasses,
value: this.userHourglasses,
});
if (this.neededCurrencyOnly === true) {
Copy link
Member

Choose a reason for hiding this comment

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

You don't need the equivalency operator, if (this.neededCurrencyOnly) { is sufficient

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, derp!

currencies.push({
type: 'gems',
Copy link
Member

Choose a reason for hiding this comment

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

This should not be hard-coded. We might want to display Hourglasses only, or Gold only, in other places.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I know. Need to figure it out!

icon: this.icons.gem,
value: this.userGems,
});
} else {
currencies.push({
type: 'hourglasses',
icon: this.icons.hourglasses,
value: this.userHourglasses,
});

currencies.push({
type: 'gems',
icon: this.icons.gem,
value: this.userGems,
});
currencies.push({
type: 'gems',
icon: this.icons.gem,
value: this.userGems,
});

currencies.push({
type: 'gold',
icon: this.icons.gold,
value: this.userGold,
});
currencies.push({
type: 'gold',
icon: this.icons.gold,
value: this.userGold,
});

for (const currency of currencies) {
if (
currency.type === this.currencyNeeded
&& !this.enoughCurrency(this.currencyNeeded, this.amountNeeded)
) {
currency.notEnough = true;
for (const currency of currencies) {
Copy link
Member

Choose a reason for hiding this comment

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

You're skipping this code block in the needed-only case. Don't we still want the component to notice when the user doesn't have enough of the needed currency, even if we're only showing one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Still thinking this one through.

if (
currency.type === this.currencyNeeded
&& !this.enoughCurrency(this.currencyNeeded, this.amountNeeded)
) {
currency.notEnough = true;
}
}
}

return currencies;
},
},
Expand Down
4 changes: 4 additions & 0 deletions website/client/src/pages/settings/components/yourBalance.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
class="balance-info"
:currency-needed="currencyNeeded"
:amount-needed="amountNeeded"
neededCurrencyOnly
Copy link
Member

Choose a reason for hiding this comment

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

huh, does this work as a destructured syntax? Do we set up props like this anywhere else? I'd lean toward keeping the style consistent for readability, tbh, given you've used our more common format even within this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, which is weird. I have, however, rewritten it so it matches our more common format.

/>
</div>
</template>
Expand All @@ -28,6 +29,9 @@ export default {
amountNeeded: {
type: Number,
},
neededCurrencyOnly: {
type: Boolean,
},
},
};
</script>
Expand Down
7 changes: 7 additions & 0 deletions website/client/src/pages/settings/settingRows/classSetting.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@
<your-balance
:amount-needed="amountNeeded"
currency-needed="gems"
:neededCurrencyOnly="true"
class="currency-align"
/>
</div>
</td>
Expand All @@ -80,6 +82,11 @@ input {
margin-right: 2rem;
}

.currency-align {
display: flex;
align-items: center;
Copy link
Member

Choose a reason for hiding this comment

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

There are helper classes for these. You don't need to add more CSS.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed, but still figuring it out

}

.form-group {
margin-bottom: 1.5rem;
}
Expand Down
Loading