-
Notifications
You must be signed in to change notification settings - Fork 125
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: adding new free shipping bar functionality #177
base: main
Are you sure you want to change the base?
feat: adding new free shipping bar functionality #177
Conversation
@lucis @guifromrio hey guys, could you look? sorry for mistakes, it's my first time working with deco.cx |
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.
Great job! Amazing!
Thanks for contributing!
export interface Props { | ||
value?: number; | ||
} |
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.
since this component will likely be used by a huge amount of people, I think it may worth to add at least short descriptions here
e.g.: https://github.com/deco-sites/fashion/blob/main/components/header/Header.tsx#L34
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.
@mcandeia ahh, yes... great suggestion
{progress < 100 | ||
? `Faltam R$ ${ | ||
formatPrice(value - total?.value / 100, currencyCode!, locale) | ||
} para o frete grátis` | ||
: "Parabéns! Você tem frete grátis"} |
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.
How about use a config key instead?
you can let the user input a sort of templatestring
e.g
export interface Messages {
leftTofreeShippingTemplate: string // "Faltam {currency} {value} para o frete grátis"
freeShipping: string // "Parabéns! Você tem frete grátis"
}
export interface Props {
value?: number;
messages: Messages;
}
Then you can replace when using it
const leftToShipping = props.message.leftToShippingTemplate.replace("{currency}", currencyCode).replace("{value}", formatPrice(value - total?.value / 100, currencyCode!, locale))
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.
@mcandeia makes a lot of sense... improvements completed
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.
Do not merge yet, I’ll fix the namespace bug that makes the namespace be your fork repo
export interface Messages { | ||
/** | ||
* @title Default message | ||
* @description Default message displayed when I don't have free shipping |
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.
* @description Default message displayed when I don't have free shipping | |
* @description The *template* that will be used for mounting the free shipping message when its not reached |
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.
Please, can you add an example with @example as I mentioned previously? Thanks
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.
Please, can you add an example with @example as I mentioned previously? Thanks
yes, done
|
||
/** | ||
* @title Free shipping message | ||
* @description Message displayed when I have free shipping |
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.
* @description Message displayed when I have free shipping | |
* @description The message used when free shipping is reached |
strokeWidth={0} | ||
/> | ||
{progress < 100 | ||
? defaultMessage.replace( |
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.
Why not replace {currency} as well?
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.
Why not replace {currency} as well?
the formatPrice() function already delivers the formatted price respecting the currency... there is no need to substitute in currency.
formatPrice() ... R$ X,XX
import_map.json
Outdated
@@ -1,5 +1,6 @@ | |||
{ | |||
"imports": { | |||
"carlosviniciusananias/fashion/": "./", |
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.
Would you mind to remove it manually? Looks like you’re working in your own fork, which is fine, but there’s a bug that adds your repistory here so you need to remove it manually.
note that you should do it as a last task since your “deno task start” will add it again
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.
Do not merge yet, I’ll fix the namespace bug that makes the namespace be your fork repo
84ccb1b
to
cb712c6
Compare
b25943b
to
daff60d
Compare
What is this contribution about?
Implementation of a free shipping bar in minicart addressing issue #112
Extra info