Follow-up from "Migrate OrderSummary component to GraphQL"
The following discussions from !59963 (merged) should be addressed:
-
@vitallium started a discussion: (+1 comment) @pgascouvaillancourt could you review please?
-
@pgascouvaillancourt started a discussion: nitpick: Instantiating a
Date
object without parameter should return the current date. Could we simplify this a bit?return new Date();
-
@pgascouvaillancourt started a discussion: nitpick: AFAICT, both
startDate
andendDate
are computed only once and don't depend on any property that might change at runtime. So I feel like they don't need to be computed properties. Perhaps we could define them in the componentsdata
, or even in its$options
? -
@pgascouvaillancourt started a discussion: nitpick: I'm seeing this casing for the first time for importing a GraphQL query in GitLab's codebase. I'd be inclined to change it to align with the rest of the codebase.
import stateQuery from 'ee/subscriptions/graphql/queries/state.query.graphql';
-
@pgascouvaillancourt started a discussion: nitpick: It looks like we don't need to set the
state
property at all because we manually set individual properties in theresult
hook. Should we set this query tomanual
and remove theupdate
hook?manual: true,
-
@pgascouvaillancourt started a discussion: nitpick: Could we move this to a computed property?
-
@pgascouvaillancourt started a discussion: nitpick: Could we leverage
GlCollapse
here? I'm suggesting this blindly because I'm not sure how to test this feature locally. I got theBuy additional minutes
to show in my instance, but it points tohttps://customers.stg.gitlab.com/buy_pipeline_minutes
🤷 -
@pgascouvaillancourt started a discussion: nitpick: We don't seem to use
taxRate
inOrderSummary
. Would it make sense to importTAX_RATE
inSummaryDetails
directly, and to remove thetax-rate
prop? -
@pgascouvaillancourt started a discussion: nitpick: We seem to be missing some test coverage for this branch.
-
@pgascouvaillancourt started a discussion: nitpick: The collapsible behavior doesn't seem to be tested.