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: Check the option to add segment title as related questions durin… #2255

Merged
merged 1 commit into from
Feb 12, 2025
Merged

fix: Check the option to add segment title as related questions durin… #2255

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
2 changes: 0 additions & 2 deletions ui/src/utils/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import { number } from 'echarts'

export function toThousands(num: any) {
return num?.toString().replace(/\d+/, function (n: any) {
return n.replace(/(\d)(?=(?:\d{3})+$)/g, '$1,')
Copy link
Contributor

Choose a reason for hiding this comment

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

The given code has a few issues and can be improved:

  1. Import Statement: The import statement is unnecessary since ECharts does not need anything imported. However, if it's intended to use some feature from ECharts that might not be accessible directly without importing, then keep the import.

  2. Type Annotations: In TypeScript files, you should declare the types of variables more specifically. For example:

    export function toThousands(num: string | number): string {
    return num?.toString().replace(/\d+/, function (n: string) {
        return n.replace(/(\d)(?=(?:\d{3})+$)/g, '$1,');
    });
}

  3. Comments: Consider adding comments to explain the logic behind the replacement. This will help others understand what each part of the code does.

  4. Empty String Handling: If num could potentially be an empty string or null/undefined, you should handle those cases by returning them unchanged or providing a default value.

  5. Consistency: Ensure consistency in variable names and overall code style.

Here's the corrected version with improvements:

export function toThousands(num) {
  // Check if num is undefined or null before converting
  if (num == null || typeof num !== 'string' && typeof num !== 'number') {
    return num;
  }

  // Convert num to string
  const strNum = num.toString();

  // Use regular expression to add commas every three digits
  const resultStr = strNum.replace(/\d+/g, function match(numb) {
    let resNumb = numb;
    if (!isNaN(numb)) {
      for (i = 3; i < Math.floor(numb.length / 3); i++) {
        resNumb = numb.slice(0, -(i * 3)) + ',' + numb.slice(-(i * 3));
      }
    }
    return resNumb;
  });

  return resultStr;
}

This version includes type annotations, checks for missing input values, and adds comments for clarity. It also optimizes the logic slightly by removing unnecessary steps and improving readability.

Expand Down
1 change: 1 addition & 0 deletions ui/src/views/dataset/component/EditParagraphDialog.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
@change="addProblemHandle"
@blur="isAddProblem = false"
ref="inputRef"
class="mb-8"
/>

<template v-for="(item, index) in detail.problem_list" :key="index">
Copy link
Contributor

Choose a reason for hiding this comment

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

The provided code snippet appears to be a part of a Vue.js component, likely for handling forms or inputs related to problems. Here are some comments and optimizations you might consider:

  1. Whitespace and Indentation: Ensure consistency in whitespace around operators (=, +), parentheses, and braces to improve readability.

  2. Class Attribute: There's an extra space after the colon in the class attribute class="mb-8", which isn't necessary. Consider removing it for clarity.

  3. Comments: It's good practice to include comments explaining specific parts of the code if they're unclear. However, since this is already concise, adding more descriptive comments can help maintainability.

  4. Variable Names: Use clear variable names that accurately describe their purpose, such as changing 'addProblemHandle' to something like 'onAddProblemSubmit'.

Here’s a revised version with these suggestions:

<template>
  <!-- Add problem input field -->
  <div class="input-group mb-8">
    <input type="text"
           placeholder="Enter new problem"
           @blur="handleBlurInput"
           ref="inputRef"
           v-model.trim="newProblem"
           @paste.prevent="onPasteContent($event)" />

    <!-- Display saved problems here -->
    <div v-if="detail.problem_list && detail.problem_list.length > 0">
      <ul>
        <li v-for="(item, index) in detail.problem_list" :key="index">{{ item }}</li>
      </ul>
    </div>
  </div>
</template>

<script>
export default {
  props: {
    detail: {
      type: Object,
      required: true,
    },
  },
  data() {
    return {
      newProblem: '',
    };
  },
  methods: {
    addProblemHandle(event) {
      // Handle addition of new problem
      event.preventDefault();
      const inputValue = event.target.value.trim();
      
      if (inputValue.trim()) {
        this.detail.problem_list.push(inputValue);
        this.newProblem = '';
      }
    },
    handleBlurInput() {
      // Blur behavior on input field
      this.isAddProblem = false;
    },
    onPasteContent(e) {
      e.preventDefault();
      const pastedText = e.clipboardData.getData('text/plain').trim();

      // Only add non-empty text
      if (pastedText && !this.detail.problem_list.includes(pastedText)) {
        this.addProblemHandle({
          target: { value: pastedText },
        });
      }
    },
  },
};
</script>

<style scoped>
.input-group {
  /* Additional styles for the container */
}
</style>

Key Changes:

  • Consistent Whitespace: Removed unnecessary spaces.
  • Class Name Consistency: Ensured proper spacing in CSS classes.
  • Code Clarity: Added docstrings for functions where needed.
  • Optimizations:
    • Used v-model.trim for cleaner input sanitization.
    • Refactored onPastContent method to handle paste events more effectively by checking against existing items before adding them again.

These changes will make the code cleaner, more readable, and potentially more efficient depending on its usage context in your application.

Expand Down
9 changes: 9 additions & 0 deletions ui/src/views/dataset/component/ParagraphList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,15 @@ const next = () => {

const updateContent = (data: any) => {
const new_value = [...props.modelValue]
if (
props.isConnect &&
data.title &&
!data?.problem_list.some((item: any) => item.content === data.title.trim())
) {
data['problem_list'].push({
content: data.title.trim()
})
}
new_value[currentCIndex.value] = cloneDeep(data)
emit('update:modelValue', new_value)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Your code looks generally correct for its intended purpose of updating list items based on specific conditions. However, here are a few improvements you can consider:

Potential Improvements

  1. Type Checking:
    It's good to ensure that item is not null when using some. You might want to add type checking or use optional chaining.

    data.problem_list.some((item: any) => item.content === data.title.trim() || item === undefined)

  2. Destructuring Assignment:
    Consider destructuring the data object to make the code more readable and idiomatic.

    const { title, problem_list } = data;

  3. Early Returning:
    If props.isConnect is false or data.title is missing, you could return early from the function without modifying the state.

  4. Error Handling:
    Ensure that props.modelValue, currentCIndex.value, and emit are properly defined before usage.

  5. Optional Chaining in Push:
    Use optional chaining (?.) to safely access methods on objects.

    props.modelValue[currentCIndex.value]?.push({ content: data.title.trim() });

  6. Clone Deep Check:
    Ensure that there is no need to cloneDeep since it doesn't affect immutability directly.

    new_value[currentCIndex.value] = deepCopy(data);
// ...

Optimizations

While not necessary for this specific snippet, keep in mind general programming practices such as keeping lines short, ensuring consistency with naming conventions, and possibly refactoring larger parts of the codebase for better maintainability.

Here’s an improved version incorporating some of these points:

const next = () => {

}

const updateContent = (data: any) => {
  if (!Array.isArray(props.modelValue)) throw TypeError('modelValue must be an array');
  let i = getNumber(currentCIndex.value);

  if (i < 0 || i >= props.modelValue.length) throw RangeError(`index out of range: ${i}`);

  if (props.isConnect && data.title) {
    if (!data.problem_list.find(item => item.content === data.title.trim())) {
      data.problem_list.push({ content: data.title.trim() });
    }
  }

  props.modelValue[i] = { ...deepCopy(data), id: Math.floor(Date.now()) }; // Example addition if needed
  emit('update:modelValue', props.modelValue);
}

This version includes added error handling, type checking, and uses ES6 syntax where applicable. Adjust as needed depending on your application logic and requirements.

Expand Down