-
-
Notifications
You must be signed in to change notification settings - Fork 13
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: add price filter/order on ProductDetail, LocationDetail & UserDetail pages #358
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool ! left only wording/cosmetic comments
src/views/LocationDetail.vue
Outdated
<v-col> | ||
<v-menu> | ||
<template v-slot:activator="{ props }"> | ||
<v-btn v-bind="props" size="small" class="mr-2" prepend-icon="mdi-filter-variant" :active="!!priceFilter">{{ $t('BrandDetail.Filter') }}</v-btn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BrandDetail.Filter : what's the good practice when there are "common" translation keys. should it be duplicated ? or under a 'Common.Filter' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to change this! We should have a common translation indeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issue when renaming a translation key, is that all existing translations are lost, no ? So maybe duplicate ? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems there is a "Translation Memory" feature:
https://support.crowdin.com/enterprise/common-questions-translators/#can-i-use-project-translation-memory-where-do-i-find-it
So if I rename the key, Crowdin should add all translations from memory to the renamed key.
src/constants.js
Outdated
@@ -10,11 +10,18 @@ export default { | |||
PRODUCT_FILTER_LIST: [ | |||
{ key: 'hide_price_count_gte_1', value: 'Hide products with prices' }, | |||
], | |||
PRICE_FILTER_LIST: [ | |||
{ key: 'show_last_month', value: 'Show Prices from Last 30 Days' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ key: 'show_last_month', value: 'Show Prices from Last 30 Days' }, | |
{ key: 'only_last_30d', value: 'Only prices for the last 30 days' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw these order / filter labels are not translated (seperate PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I wanted to discuss on this first since it would mean adding translation in constant.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or doing something similar as routes.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will create another PR
src/views/LocationDetail.vue
Outdated
if (this.priceFilter === 'show_last_month') { | ||
let oneMonthAgo = new Date() | ||
oneMonthAgo.setMonth(oneMonthAgo.getMonth() - 1) | ||
defaultParams['date__gte'] = oneMonthAgo.toISOString().split('T')[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the code elsewhere we used toISOString().substring(0, 10)
. better ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on a 1000000 loop
split: 318.093994140625 ms
substring: 3.641845703125 ms
split: 264.632080078125 ms
substring: 0.674072265625 ms
substring is indeed faster (300 nano second compared to 1ns). But both are still super fast. I guess it is down to readability. Both are fine by me, though we should try to stay uniform in the code (unless we start looping like crazy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't thinking about speed. Just uniformity 👍
What
Screenshot
Fixes bug
Closes #343