Add more options for customizations and translations#52
Conversation
|
Also a lot of package updates during npm install. But that was just a bonus. There are still a lot of major versions needed to be updated etc. |
| private ticket: CartTicket; | ||
| private seatPriceTd: HTMLTableCellElement; | ||
| private currency: string; | ||
| private currencyBehind: boolean; |
There was a problem hiding this comment.
A better alternative and naming for this property could be:
private currencyPosition: 'start' | 'end';
|
|
||
| const { cart } = store.getOptions(); | ||
| this.currency = cart?.currency || DEFAULT_CURRENCY; | ||
| this.currencyBehind = cart?.currencyBehind ?? false; |
There was a problem hiding this comment.
For consistency I would create a constant DEFAULT_CURRENCY_POSITION and set it to 'start', so this would be:
this.currencyPosition = cart?.currencyPosition || DEFAULT_CURRENCY_POSITION;
| return `${this.currency}${price.toFixed(2)}`; | ||
| return this.currencyBehind | ||
| ? `${price.toFixed(2)}${this.currency}` | ||
| : `${this.currency}${price.toFixed(2)}`; |
There was a problem hiding this comment.
You can format the price once before creating the string:
const formattedValue = price.toFixed(2);
return this.currencyPosition === 'start'
? `${this.currency}${formattedValue}`
: `${formattedValue}${this.currency}`;
|
|
||
| const { cart } = this.store.getOptions(); | ||
| this.currency = cart?.currency || DEFAULT_CURRENCY; | ||
| this.currencyBehind = cart?.currencyBehind ?? false; |
There was a problem hiding this comment.
Same improvements for currencyBehind as mentioned for CartItem.
| this.element.textContent = `Total: ${this.currency}${total.toFixed(2)}`; | ||
| const totalString = this.currencyBehind | ||
| ? `${total.toFixed(2)}${this.currency}` | ||
| : `${this.currency}${total.toFixed(2)}` |
There was a problem hiding this comment.
Also here I would create formattedValue before building the string.
| const description = `${seatType.label} (${currency}${seatType.price})`; | ||
| const currencyLocation = cart?.currencyBehind | ||
| ? `${seatType.price}${currency}` | ||
| : `${currency}${seatType.price}`; |
There was a problem hiding this comment.
currencyLocation naming can be improved here to formattedCurrency.
| titleLabel?: string; | ||
| totalLabel?: string; | ||
| currencyBehind?: boolean; | ||
| showTotal?: boolean; |
There was a problem hiding this comment.
showTotal was added but it's not used anywhere.
| submitLabel?: string; | ||
| titleLabel?: string; | ||
| totalLabel?: string; | ||
| currencyBehind?: boolean; |
There was a problem hiding this comment.
As mentioned above this would become:
currencyPosition: 'start' | 'end';
There was a problem hiding this comment.
I see you probably used a newer version of npm and this has lockfileVersion 3. However I wouldn't update it in this PR, so it would be nice if you could revert it to original.
|
Hi @simonlerpard, thank you very much for contributing! I left a few comments here and there to improve namings and options structure. |
|
Awesome @omahili, I kind of did this in a rush... (without actually thinking it through all the way 😜) I've just quickly looked at your comments and they are very reasonable. I'll look into this further soon. Hopefully during the weekend or the next. I'll update the PR again when it's done. |
|
No worries @simonlerpard! For the labels I'm actually thinking if it makes more sense to have a full object in the options for locales like: And possibly a function to set locale like:
So for now I removed all comments regarding labels. I will evaluate the solution mentioned here and eventually create another PR. I will let you know anyway! |
2 participants
Adding multiple options to improve customizations and language adoptions,
as well as the currency position.
It's no breaking change, simply just more options 😃