Generalize the User Feedback component
The following discussion from !128906 (merged) should be addressed:
-
@iamphill started a discussion: (+1 comment) I wonder if instead of including this here we should add this to https://gitlab.com/gitlab-org/gitlab/-/blob/94c995c50366b1d75b19629179298816c9dbb345/ee/app/assets/javascripts/ai/components/ai_genie.vue#L256
There are other places this component is used and I am concerned we end up adding extra padding where it isn't needed
🤔
At the moment, the User Feedback component includes logic, specific to some external component (https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/assets/javascripts/ai/components/user_feedback.vue#L41-43). This issue suggests to remove this dependency on a particular implementation.
Possible solutions:
- Extend the UserFeedback compnet to accept the new prop
customClass, for example, which will specify a class
diff --git a/ee/app/assets/javascripts/ai/components/user_feedback.vue b/ee/app/assets/javascripts/ai/components/user_feedback.vue
index 9f7203bda8bf..88f6bb37878b 100644
--- a/ee/app/assets/javascripts/ai/components/user_feedback.vue
+++ b/ee/app/assets/javascripts/ai/components/user_feedback.vue
@@ -31,17 +31,17 @@ export default {
required: false,
default: i18n.GENIE_CHAT_FEEDBACK_LINK,
},
+ customClass: {
+ type: String,
+ required: false,
+ default: '',
+ },
},
data() {
return {
feedbackReceived: false,
};
},
- computed: {
- isMRSummaryNote() {
- return this.feedbackLinkText !== i18n.GENIE_CHAT_FEEDBACK_LINK;
- },
- },
methods: {
trackFeedback({ feedbackOptions, extendedFeedback } = {}) {
this.track(this.eventName, {
@@ -67,7 +67,9 @@ export default {
<template>
<div>
- <div :class="{ 'gl-pt-4': !isMRSummaryNote }">
+ <div
+ class="gl-pt-4"
+ :class="customClass">
<gl-button v-if="!feedbackReceived" variant="link" @click="$refs.feedbackModal.show()">{{
feedbackLinkText
}}</gl-button>
diff --git a/ee/app/assets/javascripts/merge_requests/components/summary_note.vue b/ee/app/assets/javascripts/merge_requests/components/summary_note.vue
index bc695bca5763..26ac360de9fa 100644
--- a/ee/app/assets/javascripts/merge_requests/components/summary_note.vue
+++ b/ee/app/assets/javascripts/merge_requests/components/summary_note.vue
@@ -61,6 +61,7 @@ export default {
<template #feedback>
<user-feedback
event-name="proposed_changes_summary"
+ custom-class="gl-pt-0!"
:feedback-link-text="__('Leave feedback')"
/>
</template>
- Move responsibility of controlling the class all the way to the component consumers:
diff --git a/ee/app/assets/javascripts/ai/components/user_feedback.vue b/ee/app/assets/javascripts/ai/components/user_feedback.vue
index 9f7203bda8bf..0a4feb192e87 100644
--- a/ee/app/assets/javascripts/ai/components/user_feedback.vue
+++ b/ee/app/assets/javascripts/ai/components/user_feedback.vue
@@ -37,11 +37,6 @@ export default {
feedbackReceived: false,
};
},
- computed: {
- isMRSummaryNote() {
- return this.feedbackLinkText !== i18n.GENIE_CHAT_FEEDBACK_LINK;
- },
- },
methods: {
trackFeedback({ feedbackOptions, extendedFeedback } = {}) {
this.track(this.eventName, {
@@ -67,7 +62,7 @@ export default {
<template>
<div>
- <div :class="{ 'gl-pt-4': !isMRSummaryNote }">
+ <div>
<gl-button v-if="!feedbackReceived" variant="link" @click="$refs.feedbackModal.show()">{{
feedbackLinkText
}}</gl-button>
diff --git a/ee/app/assets/javascripts/ai/tanuki_bot/components/app.vue b/ee/app/assets/javascripts/ai/tanuki_bot/components/app.vue
index 0137fbf2f9cf..bbb8fd4d25e3 100644
--- a/ee/app/assets/javascripts/ai/tanuki_bot/components/app.vue
+++ b/ee/app/assets/javascripts/ai/tanuki_bot/components/app.vue
@@ -227,7 +227,7 @@ export default {
</li>
</ul>
</div>
- <div class="gl-display-flex gl-align-items-flex-end gl-mt-4">
+ <div class="gl-display-flex gl-align-items-flex-end gl-mt-6">
<user-feedback
:event-name="$options.trackingEventName"
:prompt-location="promptLocation"
The second solution might require updating all of the UserFeedback consumers which is not optimal and introduced a scope broader than needed for an MR. The first solution introduces one more reactive prop which doesn't change over time and, hence, should not be a prop.
So, as an alternative solution, it is suggested to make it as boring as possible by moving the CSS class to the outer element of the UserFeedback component to allow for the component's consumers to override it:
diff --git a/ee/app/assets/javascripts/ai/components/user_feedback.vue b/ee/app/assets/javascripts/ai/components/user_feedback.vue
index 9f7203bda8bf..b24adee232f2 100644
--- a/ee/app/assets/javascripts/ai/components/user_feedback.vue
+++ b/ee/app/assets/javascripts/ai/components/user_feedback.vue
@@ -37,11 +37,6 @@ export default {
feedbackReceived: false,
};
},
- computed: {
- isMRSummaryNote() {
- return this.feedbackLinkText !== i18n.GENIE_CHAT_FEEDBACK_LINK;
- },
- },
methods: {
trackFeedback({ feedbackOptions, extendedFeedback } = {}) {
this.track(this.eventName, {
@@ -66,8 +61,8 @@ export default {
</script>
<template>
- <div>
- <div :class="{ 'gl-pt-4': !isMRSummaryNote }">
+ <div class="gl-pt-4">
+ <div>
<gl-button v-if="!feedbackReceived" variant="link" @click="$refs.feedbackModal.show()">{{
feedbackLinkText
}}</gl-button>
diff --git a/ee/app/assets/javascripts/merge_requests/components/summary_note.vue b/ee/app/assets/javascripts/merge_requests/components/summary_note.vue
index bc695bca5763..6545e6e44cbc 100644
--- a/ee/app/assets/javascripts/merge_requests/components/summary_note.vue
+++ b/ee/app/assets/javascripts/merge_requests/components/summary_note.vue
@@ -60,6 +60,7 @@ export default {
</template>
<template #feedback>
<user-feedback
+ class="gl-pt-0!"
event-name="proposed_changes_summary"
:feedback-link-text="__('Leave feedback')"
/>