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.

Sign up for GitHub

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

Jump to bottom

feat: new sellingPrice value considering unit multiplier and tax percentage for product price #114

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

feat: new sellingPrice value considering unit multiplier and tax percentage for product price #114

wants to merge 9 commits into from

Conversation

iago1501
Copy link
Contributor

@iago1501 iago1501 commented Jan 4, 2023

What problem is this solving?

Adding a new type of selling price

How to test it?

[Workspace](Link goes here!)

Screenshots or example usage:

Describe alternatives you've considered, if any.

Related to / Depends on

How does this PR make you feel? 🔗

![](put .gif link here - can be found under "advanced" on giphy)

@iago1501 iago1501 added the enhancement New feature or request label Jan 4, 2023
@iago1501 iago1501 requested review from a team as code owners January 4, 2023 14:41
@iago1501 iago1501 self-assigned this Jan 4, 2023
@iago1501 iago1501 requested a review from a team as a code owner January 4, 2023 14:41
@vtex-io-ci-cd
Copy link

vtex-io-ci-cd bot commented Jan 4, 2023 •
edited by icazevedo
Loading

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch (backwards-compatible bug fixes)

  • Minor (backwards-compatible functionality)

  • Major (incompatible API changes)

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

@iago1501 iago1501 marked this pull request as draft January 4, 2023 14:43
@mihainutiu-vtex
Copy link

Hi @iago1501
I did a test with your branch and noticed the calculation was not correct. I made a change and commited it here.
I also published a hkignore I tested with and it works correctly.
[email protected]

@mihainutiu-vtex mihainutiu-vtex marked this pull request as ready for review March 10, 2023 09:31
icazevedo
icazevedo reviewed Jul 11, 2023
View reviewed changes
Copy link
Contributor

@icazevedo icazevedo left a comment

Choose a reason for hiding this comment

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

LGTM

react/yarn.lock Outdated Show resolved Hide resolved
.eslintrc Outdated Show resolved Hide resolved
mihainutiu-vtex
mihainutiu-vtex approved these changes Jul 12, 2023
View reviewed changes
Copy link

@mihainutiu-vtex mihainutiu-vtex left a comment

Choose a reason for hiding this comment

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

LGTM.

@Fbenitez97
Copy link

Hello! @icazevedo do you think we can merge?

@marcelovicentegc
Copy link
Member

I'm closing this PR since it is 6 month old. If this is still relevant, feel free to reopen it.

@mihainutiu-vtex
Copy link

Hi @marcelovicentegc
Yes, it is still relevant and we were waiting for it to be approved. I had published a hkignore a year ago to unblock the client but we need this new type of price definition to be available.
Please review and merge.

@iago1501 iago1501 closed this Jul 4, 2024
@mihainutiu-vtex
Copy link

Hi @iago1501
Why was this PR closed? We still need this functionality, as I mentioned previously I had published a hkignore for the specific customer that was impacted, but it would be really great if this variable was added in the app so that the client can benefit from any further enhancements and not be stuck on the hkignore version.

🙏🏼

@mihainutiu-vtex
Copy link

Hi @iago1501 Can you please review this PR as well as #121
We have a customer that needs these new price variables. I had published a beta version but there was a release today that overwrote their version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihainutiu-vtex mihainutiu-vtex mihainutiu-vtex approved these changes

@sheilagomes sheilagomes sheilagomes approved these changes

@ricardoaerobr ricardoaerobr Awaiting requested review from ricardoaerobr ricardoaerobr is a code owner automatically assigned from vtex-apps/technical-writers

@lariciamota lariciamota Awaiting requested review from lariciamota lariciamota is a code owner automatically assigned from vtex-apps/store-framework-devs

@victorhmp victorhmp Awaiting requested review from victorhmp victorhmp is a code owner automatically assigned from vtex-apps/store-framework-devs

@RodrigoTadeuF RodrigoTadeuF Awaiting requested review from RodrigoTadeuF RodrigoTadeuF is a code owner automatically assigned from vtex-apps/store-framework-devs

@icazevedo icazevedo Awaiting requested review from icazevedo

At least 1 approving review is required to merge this pull request.

Assignees

@iago1501 iago1501

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@iago1501 @mihainutiu-vtex @Fbenitez97 @marcelovicentegc @icazevedo @sheilagomes