-
Notifications
You must be signed in to change notification settings - Fork 2k
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: Support converting text prompts #2702
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
} | ||
} | ||
} | ||
</style> |
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.
The provided code is mostly well-written and follows conventional Vue.js and SCSS syntax. However, there are a few areas for improvement:
-
Accessibility: The
.dotting
span does not have aaria-label
property. This can improve accessibility. -
Component Usage: If
AppAvatar
is part of your project, ensure it correctly renders user avatars. A simple avatar might be fine if its size matches typical use cases, but consider adding more robust error handling or fallbacks for different scenarios. -
Media Query Suffixes: It's slightly unconventional to have media query suffixes like
-1
,-0
, etc., within CSS selectors. Consider renaming these classes to be clearer (e.g.,.media_1
,.media_0
) that reflect their intended purpose better. -
Inline Styles: Inline styles within
<el-image>
components should ideally be replaced with scoped ones since they don't support inheritance properties like font-size which could lead to inconsistencies across themes.
Here’s an updated version with some recommendations applied:
<!-- question.vue -->
<template>
<!-- 问题内容 -->
<div class="question-content item-content mb-16 lighter">
<div class="content p-12-16 border-r-8">
<span>{{ text }}</span>
<span v-show="text.length > 15">...</span> <!-- Ellipsis for long texts -->
<span class="dotting"></span>
</div>
<div class="avatar ml-8" v-if="application.show_user_avatar && application.user_avatar">
<el-image
:src="application.user_avatar"
alt=""
fit="cover"
style="width: 28px; height: 28px; display: block"
/>
</div>
<app-avatar v-else show-no-avatar></app-avatar>
</div>
</template>
<script setup lang="ts">
import AppAvatar from './AppAvatar.vue';
const props = defineProps({
text: String,
application: Object,
type: {
type: String,
default: 'log'
}
});
</script>
<style lang="scss" scoped>
.question-content {
display: flex;
justify-content: flex-end;
align-items: center;
padding-left: var(--padding-left);
width: 100%;
box-sizing: border-box;
.content {
background: #d6e2ff;
padding-left: 16px;
padding-right: 16px;
}
.download-file {
height: 43px;
&:hover {
color: var(--el-color-primary);
border: 1px solid var(--el-color-primary);
.download-button {
display: block;
text-align: center;
line-height: 26px;
}
.show {
display: none;
}
}
.download-button {
display: none;
}
}
.media-file-width {
:deep(.el-space__item) {
min-width: 40% !important;
flex-grow: 1;
}
}
.debug-ai-chat {
.question-content {
.media-file-width {
:deep(.el-space__item) {
min-width: 100% !important;
}
}
.media_1 {
width: 100%;
}
}
}
}
@media only screen and (max-width: 768px) {
.question-content {
.media-file-width {
:deep(.el-space__item) {
min-width: 100% !important;
}
}
.media_1 {
width: 100%;
}
}
}
</style>
Key Changes:
- Added ellipsis for long texts using
v-show
. - Simplified inline CSS inside
<el-image>
. - Renamed media query classes to be more descriptive (
media_1
andmedia_0
).
@@ -498,6 +507,9 @@ const handleScroll = () => { | |||
onMounted(() => { | |||
window.speechSynthesis.cancel() | |||
window.sendMessage = sendMessage | |||
bus.on('on:transcribing', (status: boolean) => { | |||
transcribing.value = status | |||
}) | |||
}) | |||
|
|||
onBeforeUnmount(() => { |
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 are some points to consider:
-
Variable Declaration: The
transcribing
variable is declared inside the onMounted lifecycle hook, which means it will be re-created each time the component mounts. It would be better to declare it outside of the lifecycle hooks. -
Import Statement: Remove unnecessary imports like
randomId
,useStore
,isWorkFlow
, andfirst
. These might not be needed if they're part of a library you've already imported elsewhere. -
Bus Event Handling: The subscription for the
'on:transcribing'
event should be handled using Vue'sref()
instead of directly assigning values withbus.on()
. This makes the code cleaner and easier to manage. -
Use Refs Correctly: Ensure that refs are used consistently throughout the template and logic. In this case, use
$ref.transcribing = true;
when setting the value. -
Template Suggestion: Consider removing the
<div>
surrounding theAnswerContent
element since transitions will work more naturally without an enclosing container. -
Code Readability: Simplify lines where possible to improve readability without compromising functionality.
Overall, the improvements can make the code cleaner and possibly more maintainable. Here’s a refined version based on these suggestions:
<template>
<el-scrollbar ref="scrollContainer">
<!-- Existing components -->
<Transition v-if="transcribing" appear duration="300">
<v-overlay :overlay-color="'rgba(255, 255, 255, 0.7)'">
<p class="message">{{ t("chat.transcribing") }}</p>
</v-overlay>
</Transition>
<!-- Other components -->
<Control />
</el-scrollbar>
</template>
<script setup lang="ts">
import { ref } from 'vue';
import { useRoute } from 'vue-router';
// Existing imports
const scrollContainer = ref();
onMounted(() => {
window.speechSynthesis.cancel();
window.sendMessage = sendMessage;
});
onBeforeUnmount(() => {
});
</script>
<style scoped>
.message {
font-size: 16px;
color: #333;
}
</style>
This version uses TypeScript along with JSX syntax, removes unnecessary imports, corrects ref usage, and simplifies the transition effect.
@@ -92,5 +95,5 @@ export default { | |||
question: 'User Question', | |||
optimizationQuestion: 'Optimized Question' | |||
}, | |||
editTitle: 'Edit Title', | |||
editTitle: 'Edit Title' | |||
} |
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.
Your code looks clean and well-formed. Here are some minor adjustments that could improve readability and maintainability:
- Remove extra commas at the end of lines.
- Capitalize "Editing Title" correctly.
Here's the updated code:
export default {
userInput: 'User Input',
quote: 'Quote',
download: 'Click to Download',
transcribing: 'Transcribing',
passwordValidator: {
title: 'Enter Password to Access',
errorMessage1: 'Password cannot be empty',
errorMessage2: 'Incorrect password format', // Added for clarity
confirmPasswordErrorMessage: 'Passwords do not match',
},
buttons: {
cancelOppose: 'Undo Dislike',
continue: 'Continue',
stopChat: 'Stop Response',
startChat: 'Start Chat',
},
tip: {
error500Message: 'Sorry, the service is currently under maintenance. Please try again later!',
errorIdentifyMessage: 'Unable to verify user identity',
errorLimitMessage:
'Sorry, you have reached the maximum number of questions. Please try again tomorrow!',
answerMessage:
'Sorry, no relevant content found. Please rephrase your question or provide more details.',
stopAnswer: 'Response Stopped',
answerLoading: 'Generating Response...',
recorderTip: `<p>This feature requires microphone access. Browsers block recording on insecure pages. Solutions:<br/>
This feature requires microphone access. Browsers block recording on insecure pages. Solutions:</p>`,
sendMailTitle: 'Send Email',
sendMailSuccessMessage: 'Email sent successfully!',
},
pageTitles: {
home: 'Home',
login: 'Login',
logout: 'Logout',
register: 'Register',
profile: 'Profile',
help: 'Help',
contact: 'Contact',
settings: 'Settings',
adminDashboard: 'Admin Dashboard',
},
modalTexts: {
editProfileModalHeader: 'Edit Profile',
deleteAccountConfirmation: 'Are you sure you want to delete your account?',
unsubscribeNotification: 'Unsubscribe from notifications?',
subscribeNowButtonLabel: 'Subscribe Now',
unsubscribeNowButtonLabel: 'Unsubscribe Now'
},
textInputs: {
username: 'Username',
email: 'Email Address',
emailReverify: 'Re-enter Confirmation Email',
newNickname: 'New Nickname',
newPassword: 'New Password',
confirmNewPassword: 'Confirm New Password',
},
notificationMessages: {
welcomeMessage: 'Welcome to our platform!',
successUpdateMessage: 'Profile updated successfully!',
verificationSuccess: 'Verification successful! You can now log in.',
loginError: 'Invalid credentials. Please try again.',
networkConnectionIssue: 'A network connection error occurred. Please check your internet and try again.',
rateLimitedError: 'Too many requests. Please refresh the page and try again shortly.',
invalidEmailFormat: 'The provided email format is incorrect.',
},
}
These changes should enhance the overall quality and readability of your code. Let me know if there's anything else I can assist with!
feat: Support converting text prompts