fix: update currency display in setting for change class #15391
Conversation
CuriousMagpie
changed the title
CuriousMagpie
changed the title
| }); | ||
| if (this.neededCurrencyOnly === true) { | ||
| currencies.push({ | ||
| type: 'gems', |
There was a problem hiding this comment.
This should not be hard-coded. We might want to display Hourglasses only, or Gold only, in other places.
There was a problem hiding this comment.
Yep, I know. Need to figure it out!
| @@ -67,33 +71,40 @@ export default { | |||
| computed: { | |||
| currencies () { | |||
| const currencies = []; | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I rewrote it so the array is defined within the const declaration instead using .push()
| @@ -11,6 +11,7 @@ | |||
| class="balance-info" | |||
| :currency-needed="currencyNeeded" | |||
| :amount-needed="amountNeeded" | |||
| neededCurrencyOnly | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It does, which is weird. I have, however, rewritten it so it matches our more common format.
| @@ -80,6 +82,11 @@ input { | |||
| margin-right: 2rem; | |||
| } | |||
|
|
|||
| .currency-align { | |||
| display: flex; | |||
| align-items: center; | |||
There was a problem hiding this comment.
There are helper classes for these. You don't need to add more CSS.
There was a problem hiding this comment.
Removed, but still figuring it out
| icon: this.icons.hourglasses, | ||
| value: this.userHourglasses, | ||
| }); | ||
| if (this.neededCurrencyOnly === true) { |
There was a problem hiding this comment.
You don't need the equivalency operator, if (this.neededCurrencyOnly) { is sufficient
| && !this.enoughCurrency(this.currencyNeeded, this.amountNeeded) | ||
| ) { | ||
| currency.notEnough = true; | ||
| for (const currency of currencies) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Still thinking this one through.
Currently, when a user wishes to change their class on the Settings page, it shows hourglasses, gems, and gold. Since the currency used to change class is gems, this fix to add extra logic and a new prop was required. A small CSS change was required as well.