Skip to content
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.

Sign up for GitHub

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

fix: Display setting bugs #2258

Merged
merged 1 commit into from
Feb 12, 2025
Merged

fix: Display setting bugs #2258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions ui/src/views/application-overview/component/DisplaySettingDialog.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ const emit = defineEmits(['refresh'])

const displayFormRef = ref()
const form = ref<any>({
show_source: false
show_source: false,
language: '',
})

const detail = ref<any>(null)
Expand All @@ -70,18 +71,15 @@ const loading = ref(false)
watch(dialogVisible, (bool) => {
if (!bool) {
form.value = {
show_source: false
show_source: false,
language: '',
}
}
})
const open = (data: any, content: any) => {
detail.value = content
form.value.show_source = data.show_source
form.value.language = data.language
if (!form.value.language) {
form.value.language = getBrowserLang()
}

form.value.language = data.language || getBrowserLang()
dialogVisible.value = true
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There appear to be no apparent technical issues with the provided code. However, there are a few areas where improvements can make the logic more robust and user-friendly:

  1. Language Defaulting: The default case for language being set to getBrowserLang() is unnecessary when you have an initial value defined ('': ''). It's better to remove this redundant line since it defaults language immediately.

  2. Code Formatting: Although not required, formatting the code consistently could improve readability. Use consistent spacing and indentation patterns.

  3. Variable Initialization: Initializing dialogVisible to false, even though its setter function resets it to true. This seems superfluous unless there's specific behavior expected that would change depending on previous values.

  4. Comments Clarity: While current comments are clear, they might benefit from minor clarification or removal if the intent is straightforward.

Here's a revised version of the code incorporating these suggestions:

const emit = defineEmits(['refresh'])

const displayFormRef = ref()
const form = ref({
  show_source: false,
  language: '', // Initial/default value
})

const detail = ref(null)
const loading = ref(false)

watch(dialogVisible, (bool) => {
  if (!bool) {
    resetFormValues();
  }
});

function resetFormValues() {
  form.value.show_source = false;
  form.value.language = '';
}

const open = (data: any, content: any) => {
   detail.value = content;
   form.value.show_source = data.show_source;

   // Default language if none given
   form.value.language ||= getBrowserLang();

   dialogVisible.value = true;
}

Key Changes:

  • Removed redundant setting of language.
  • Refactored variable initialization inside the watcher callback.
  • Added resetFormValues helper function to reduce redundancy in handling form reset.
  • Used logical OR assignment operator (+=) or short-circuit evaluation || for default values.

Expand Down
40 changes: 17 additions & 23 deletions ui/src/views/application-overview/component/XPackDisplaySettingDialog.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,16 +146,16 @@
</div>
</div>

<el-form ref="displayFormRef" :model="form">
<el-form ref="displayFormRef" :model="xpackForm">
<el-row class="w-full mb-8">
<el-col :span="12">
<h5 class="mb-8">
{{ $t('views.applicationOverview.appInfo.SettingDisplayDialog.customThemeColor') }}
</h5>
<div>
<el-color-picker v-model="form.custom_theme.theme_color" />
<el-color-picker v-model="xpackForm.custom_theme.theme_color" />
{{
!form.custom_theme.theme_color
!xpackForm.custom_theme.theme_color
? $t('views.applicationOverview.appInfo.SettingDisplayDialog.default')
: ''
}}
Expand All @@ -167,14 +167,14 @@
$t('views.applicationOverview.appInfo.SettingDisplayDialog.headerTitleFontColor')
}}
</h5>
<el-color-picker v-model="form.custom_theme.header_font_color" />
<el-color-picker v-model="xpackForm.custom_theme.header_font_color" />
</el-col>
</el-row>
<el-row class="w-full mb-8">
<h5 class="mb-8">
{{ $t('views.applicationOverview.appInfo.SettingDisplayDialog.languageLabel') }}
</h5>
<el-select v-model="form.language" clearable>
<el-select v-model="xpackForm.language" clearable>
<el-option
v-for="item in langList"
:key="item.value"
Expand Down Expand Up @@ -256,7 +256,7 @@
$t('views.applicationOverview.appInfo.SettingDisplayDialog.iconDefaultPosition')
}}</span>
<el-checkbox
v-model="form.draggable"
v-model="xpackForm.draggable"
:label="
$t('views.applicationOverview.appInfo.SettingDisplayDialog.draggablePosition')
"
Expand All @@ -265,7 +265,7 @@
<el-row :gutter="8" class="w-full mb-8">
<el-col :span="12">
<div class="flex align-center">
<el-select v-model="form.float_location.x.type" style="width: 80px">
<el-select v-model="xpackForm.float_location.x.type" style="width: 80px">
<el-option
:label="
$t(
Expand All @@ -284,7 +284,7 @@
/>
</el-select>
<el-input-number
v-model="form.float_location.x.value"
v-model="xpackForm.float_location.x.value"
:min="0"
:step="1"
:precision="0"
Expand All @@ -297,7 +297,7 @@
</el-col>
<el-col :span="12">
<div class="flex align-center">
<el-select v-model="form.float_location.y.type" style="width: 80px">
<el-select v-model="xpackForm.float_location.y.type" style="width: 80px">
<el-option
:label="
$t(
Expand All @@ -316,7 +316,7 @@
/>
</el-select>
<el-input-number
v-model="form.float_location.y.value"
v-model="xpackForm.float_location.y.value"
:min="0"
:step="1"
:precision="0"
Expand All @@ -333,30 +333,30 @@

<el-space direction="vertical" alignment="start" :size="2">
<el-checkbox
v-model="form.show_source"
v-model="xpackForm.show_source"
:label="
isWorkFlow(detail.type)
? $t('views.applicationOverview.appInfo.SettingDisplayDialog.showExecutionDetail')
: $t('views.applicationOverview.appInfo.SettingDisplayDialog.showSourceLabel')
"
/>
<el-checkbox
v-model="form.show_history"
v-model="xpackForm.show_history"
:label="$t('views.applicationOverview.appInfo.SettingDisplayDialog.showHistory')"
/>
<el-checkbox
v-model="form.show_guide"
v-model="xpackForm.show_guide"
:label="$t('views.applicationOverview.appInfo.SettingDisplayDialog.displayGuide')"
/>
<el-checkbox
v-model="form.disclaimer"
v-model="xpackForm.disclaimer"
:label="$t('views.applicationOverview.appInfo.SettingDisplayDialog.disclaimer')"
@change="changeDisclaimer"
/>
<span v-if="form.disclaimer"
><el-tooltip :content="form.disclaimer_value" placement="top">
<span v-if="xpackForm.disclaimer"
><el-tooltip :content="xpackForm.disclaimer_value" placement="top">
<el-input
v-model="form.disclaimer_value"
v-model="xpackForm.disclaimer_value"
style="width: 422px; margin-bottom: 10px"
@change="changeValue"
:maxlength="128"
Expand Down Expand Up @@ -421,9 +421,6 @@ const defaultSetting = {
}

const displayFormRef = ref()
const form = ref<any>({
show_source: false
})

const xpackForm = ref<any>({
show_source: false,
Expand Down Expand Up @@ -468,7 +465,6 @@ const customStyle = computed(() => {
})

function resetForm() {
form.value = cloneDeep(defaultSetting)
xpackForm.value = cloneDeep(defaultSetting)
imgUrl.value = {
avatar: '',
Expand Down Expand Up @@ -503,7 +499,6 @@ const open = (data: any, content: any) => {
imgUrl.value.user_avatar = data.user_avatar
xpackForm.value.disclaimer = data.disclaimer
xpackForm.value.disclaimer_value = data.disclaimer_value
console.log(xpackForm.value.disclaimer_value)
if (
xpackForm.value.disclaimer_value ===
t('views.applicationOverview.appInfo.SettingDisplayDialog.disclaimerValue')
Expand All @@ -520,7 +515,6 @@ const open = (data: any, content: any) => {
header_font_color: data.custom_theme?.header_font_color || '#1f2329'
}
xpackForm.value.float_location = data.float_location
form.value = xpackForm.value
dialogVisible.value = true
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks generally correct but has some areas that could be optimized or improved. Here are a few suggestions:

Code Areas with Issues

  1. Duplicate Data Handling: The form and xpackForm objects use the same structure (show_source, etc.), but they should be separated to avoid confusion and potential bugs. Consider removing one of them.

  2. Missing Return Statements in Conditional Rendering: In the template section, there's an unhandled case where the disclaimer might not be set, which can cause undefined behavior.

  3. Unnecessary Cloning: While cloning is necessary for initial setup, it might be redundant within the lifecycle hooks (open method). Avoid unnecessary clones unless absolutely necessary.

  4. Comments Quality: Some comments, such as those inside tooltips or input fields, could be more detailed for better clarity when reading the code later.

Optimizations/Suggestions

  1. Separate Configuration Objects: Refactor the xpackForm object to separate different configurations like themes, localization, position controls, and visibility settings.

  2. Improve Exception Handling: For cases where disclaimer_value might not be set, consider adding error handling or default values to prevent runtime errors.

  3. Cleaner Tooltip Content Logic: Simplify how tooltip content is generated using computed properties or computed ternary expressions for cleaner code.

Here’s an updated version of the relevant parts based on these suggestions:

<template>
  <!-- ... existing elements ... -->
  
  <section>
    <h5>{{ $t('views.applicationOverview.appInfo.SettingDisplayDialog.languageLabel') }}</h5>
    <el-select v-model="lang" clearable @change="handleLanguageChange">
      <el-option
        v-for="item in langList"
        :key="item.value"
        :label="item.label"
        :value="item.value"
      >
      </el-option>
    </el-select>

    
     <!-- Existing sections with logic -->
   
</template>

<script setup lang="ts">
import { ref } from 'vue';
// Import other dependencies here...

interface ThemeConfig {
  theme_color: string;
  header_font_color: string;
}

interface DisplaySettings {
  show_execution_detail: boolean;
  show_version_description: boolean;
  show_source: boolean;
  show_history: boolean;
  show_guide: boolean;
  disclaimer: boolean;
  disclaimer_value?: string;
  float_location?: {
    x: LocationConfig;
    y: LocationConfig;
  };
}

interface LocationConfig {
  type: string;
  value: number | string;
}

const displaySettings = ref<DisplaySettings>({
  show_execution_detail: true,
  show_version_description: true,
  show source: false,
  show_history: true,
  show_guide: true,
  disclaimer: false,
});

const themeConfig = ref<ThemeConfig>({
  theme_color: '#FFFFFF',
  header_font_color: '#000000',
});

function handleLanguageChange(value) {
  // Handle language change logic...
}
</script>

This approach separates configuration into manageable components, makes sure exceptions are handled appropriately, and simplifies conditional rendering through Vue's reactive system.

Expand Down