-
Notifications
You must be signed in to change notification settings - Fork 38
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(cost-analysis-l-n-b-store): create cost-analysis-l-n-b-store #1702
feat(cost-analysis-l-n-b-store): create cost-analysis-l-n-b-store #1702
Conversation
Signed-off-by: samuel.park <[email protected]>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dataSourceList: [ | ||
'aws', | ||
'azure', | ||
'gcp', | ||
], |
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.
question: this is mock data, right?
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.
oh.. yes... I forgot that.. I'll remove them.
Signed-off-by: samuel.park <[email protected]>
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~ Nice refactoring!
It seems that the $reset() and $dispose() functions of this store are not being called at all in the code. It might be worth considering whether these functions should be executed during the creation or unmounting of the LNB, if that hasn't been reviewed already.
dataSourceList: [ | ||
'aws', | ||
'azure', | ||
'gcp', | ||
], |
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.
nitpick: Please add TODO comment not to miss updating it and to notify reviewers that this is not the mistake. :)
@@ -178,15 +158,15 @@ export const useCostAnalysisPageStore = defineStore('cost-analysis-page', { | |||
name, | |||
options, | |||
}); | |||
this.selectedQueryId = createdData.cost_query_set_id; | |||
costAnalysisLNBStore.$patch({ selectedQueryId: createdData.cost_query_set_id }); |
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.
praise: Oh, I didn't know that we can use pinia store inside the other pinia store. 😮
Thank you for introducing me to a new way of using it.
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!!!
…oudforet-io#1702) Signed-off-by: samuel.park <[email protected]>
…oudforet-io#1702) Signed-off-by: samuel.park <[email protected]>
To Reviewers
style
,chore
,ci
ONLY)Type of Change
Affects to
Packages
Apps
Checklist
lint
andtype
?mirinae
should be reflected instorybook
.Description
SSIA
cost-analysis-l-n-b-store
. Add Data-Source statecost-analysis-page-store
. (tocost-analysis-l-n-b-store
)