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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move VoucherExecution component under packages/ui #129

Open
4 tasks
nevendyulgerov opened this issue Mar 7, 2024 · 2 comments
Open
4 tasks

Move VoucherExecution component under packages/ui #129

nevendyulgerov opened this issue Mar 7, 2024 · 2 comments
Assignees
Labels
Status: Discussion Collaboratively refine some idea / feature Type: Refactor Code changes that neither adds a new feature nor fix a bug.

Comments

@nevendyulgerov
Copy link
Contributor

馃搫 Context

As per this comment move the VoucherExecution component under packages/ui.

馃搱 Subtasks

  • VoucherExecution is moved to packages/ui
  • Test suite for VoucherExecution is moved to packages/ui

馃幆 Definition of Done

  • Executing a voucher works ok
  • Tests for voucher execution are passing
@nevendyulgerov nevendyulgerov added the Type: Refactor Code changes that neither adds a new feature nor fix a bug. label Mar 7, 2024
@nevendyulgerov nevendyulgerov self-assigned this Mar 7, 2024
@brunomenezes brunomenezes added the Status: Discussion Collaboratively refine some idea / feature label Mar 7, 2024
@brunomenezes
Copy link
Collaborator

Hi @nevendyulgerov, my comment was to discuss the motivation behind the current choice rather than move it directly.
For example, a few questions on that end;

  • What was the motivation to add to the apps/web instead of the packages/ui?
  • Is that because of the types generated by our codegen tools? (Generators are good if you accept that sometimes some work will be needed on rough edges).
  • Is that related to extensibility?

@nevendyulgerov
Copy link
Contributor Author

nevendyulgerov commented Mar 8, 2024

@brunomenezes , the motivation was the types coming from the codegen located in apps/web.

In VoucherExecution we are using a prop voucher with interface Voucher coming from graphql/rollups/types. Initially, my plan was to place this component under packages/ui, however due to this type already existing in apps/web I decided to keep it there to prevent interface duplication.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Discussion Collaboratively refine some idea / feature Type: Refactor Code changes that neither adds a new feature nor fix a bug.
Projects
Status: 馃搶 Todo
Development

No branches or pull requests

2 participants