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.

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

carlosviniciusananias
Copy link

What is this contribution about?

Implementation of a free shipping bar in minicart addressing issue #112

Extra info

image

@carlosviniciusananias
Copy link
Author

carlosviniciusananias commented Apr 27, 2023

@lucis @guifromrio hey guys, could you look? sorry for mistakes, it's my first time working with deco.cx

Copy link
Contributor

@mcandeia mcandeia left a 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!

Comment on lines 6 to 8
export interface Props {
value?: number;
}
Copy link
Contributor

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

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

Comment on lines 33 to 37
{progress < 100
? `Faltam R$ ${
formatPrice(value - total?.value / 100, currencyCode!, locale)
} para o frete grátis`
: "Parabéns! Você tem frete grátis"}
Copy link
Contributor

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))

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

Copy link
Contributor

@mcandeia mcandeia left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @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

Copy link
Contributor

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @description Message displayed when I have free shipping
* @description The message used when free shipping is reached

strokeWidth={0}
/>
{progress < 100
? defaultMessage.replace(
Copy link
Contributor

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?

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/": "./",
Copy link
Contributor

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

Copy link
Contributor

@mcandeia mcandeia left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants