Skip to content
This repository was archived by the owner on May 21, 2020. It is now read-only.

ref: useUserAddressFactory#381

Open
defudef wants to merge 9 commits into
masterfrom
rfc-use-user-address-factory
Open

ref: useUserAddressFactory#381
defudef wants to merge 9 commits into
masterfrom
rfc-use-user-address-factory

Conversation

@defudef
Copy link
Copy Markdown
Contributor

@defudef defudef commented Apr 14, 2020

  • I'm confirming that if i made any changes to public APIs or exposed any public APIs they are doccumented.

Comment thread packages/core/rfcs/use-user-address-factory.md Outdated
Comment thread packages/core/rfcs/use-user-address-factory.md Outdated
Comment thread packages/core/rfcs/use-user-address-factory.md Outdated
Comment thread packages/core/rfcs/use-user-address-factory.md Outdated
Comment thread packages/core/rfcs/use-user-address-factory.md Outdated
Comment thread packages/core/rfcs/use-user-address-factory.md Outdated
@defudef defudef requested review from filrak and patzick April 17, 2020 06:45
Comment thread packages/core/rfcs/use-user-address-factory.md Outdated
Comment thread packages/core/rfcs/use-user-address-factory.md Outdated
@defudef defudef changed the title [RFC] useUserAddressFactory ref: useUserAddressFactory Apr 17, 2020
// ...leave all the stuff as they are now

// needed to add this:
setUser: (newUser: USER) => void;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It shouldn't be a part of params, we have access to this object from the inside therefore we can just return this fn

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please take a look at @andrzejewsky RFC

```TS
export interface UseUserAddress<ADDRESS, SEARCH_PARAMS, ADDRESS_OPTIONS = unknown> {
addresses: ComputedProperty<ADDRESS[]>;
shippingAddresses: ComputedProperty<ADDRESS[]>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we have multiple shipping addressess and billing addresses? Does every address have to be in a specific category?

shippingAddresses: ComputedProperty<ADDRESS[]>;
billingAddresses: ComputedProperty<ADDRESS[]>;
loadAddresses: (params?: SEARCH_PARAMS) => Promise<void>;
addAddress: (address: ADDRESS, options?: ADDRESS_OPTIONS) => Promise<void>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what are these options

Copy link
Copy Markdown
Contributor

@patzick patzick left a comment

Choose a reason for hiding this comment

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

  • We'll leave only addresses, getters may filter shipping and billing addresses from it.
    There should be an additional field in address, which would be considered by getters to filter addresses of different types. ( ex __type)
  • for saving address there is only saveAddress method

Comment on lines +15 to +16
addAddress: (address: ADDRESS, options?: ADDRESS_OPTIONS) => Promise<void>;
updateAddress: (address: ADDRESS, options?: ADDRESS_OPTIONS) => Promise<void>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
addAddress: (address: ADDRESS, options?: ADDRESS_OPTIONS) => Promise<void>;
updateAddress: (address: ADDRESS, options?: ADDRESS_OPTIONS) => Promise<void>;
saveAddress: (address: ADDRESS, options?: ADDRESS_OPTIONS) => Promise<void>;

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants