-
Notifications
You must be signed in to change notification settings - Fork 315
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/LIVE-12118]: add buy sell ui flag to enable progressive replacement of multibuy-v2 live app #6686
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
28be9e2
to
44351a9
Compare
@@ -461,6 +462,7 @@ export type Feature_LldRefreshMarketData = Feature<{ | |||
|
|||
export type Feature_CounterValue = DefaultFeature; | |||
export type Feature_MockFeature = DefaultFeature; | |||
export type Feature_BuySellUi = DefaultFeature; |
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.
export type Feature_BuySellUi = DefaultFeature; | |
export type Feature_BuySellUI = DefaultFeature; |
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.
Although I agree, I am hesitant to capitalise as the other flags seem to consistently use full camelCase e.g. llmFeature
etc
@@ -105,7 +107,9 @@ export type ExchangeComponentParams = { | |||
|
|||
const Exchange = ({ match }: RouteComponentProps<ExchangeComponentParams>) => { | |||
const appId = match?.params?.appId; | |||
const buySellUiFlag = useFeature("buySellUi"); |
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.
I'd rename this to be a little clearer for other developers ... explaining that this is a rollout on v3 of Buy Sell FE, maybe something like buySellUIV3
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.
if we make the manifest id extendable then the version could be anything rather than always 3, so it might be best to avoid using the version actually..
3395b39
@@ -71,7 +74,7 @@ function getProxyURL(url: string) { | |||
// This is to handle links set in the useFromAmountStatusMessage in LLC. | |||
// Also handles a difference in paths between LLD on LLD /platform/:app_id | |||
// but on LLM /discover/:app_id | |||
if (hostname === "platform" && [DEFAULT_MULTIBUY_APP_ID].includes(platform)) { | |||
if (hostname === "platform" && [DEFAULT_MULTIBUY_APP_ID, BUY_SELL_UI_APP_ID].includes(platform)) { |
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.
Here do we want to add the buySellUiFlag.params.manifestId
in the array ?
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.
Yes for completion, good plan, added! Thanks!
9c758fb
to
6101188
Compare
6101188
to
7d797e6
Compare
7d797e6
to
0d31252
Compare
8c1b9d9
to
11592fe
Compare
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.
Looking good to me
Pinging @Justkant for reference when he comes back from holiday
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.
LGTM
✅ Checklist
npx changeset
was attached.buySellUi
{ "params": { "manifestId": "buy-sell-ui" } }
📝 Description
❓ Context
🧐 Checklist for the PR Reviewers