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.

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

refactor: inline all x-design design config #2317

Merged
merged 17 commits into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
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
26 changes: 24 additions & 2 deletions packages/design/aurora/index.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,26 @@
import Alert from './src/alert'
import BreadcrumbItem from './src/breadcrumb-item'
import CollapseItem from './src/collapse-item'
import Drawer from './src/drawer'
import Dropdown from './src/dropdown'
import DropdownMenu from './src/dropdown-menu'
import DropdownItem from './src/dropdown-item'
import FilterBox from './src/filter-box'
import Form from './src/form'
import Guide from './src/guide'
import Grid from './src/grid'
import Milestone from './src/milestone'
import Popconfirm from './src/popconfirm'
import Popover from './src/popover'
import Switch from './src/switch'
import Select from './src/select'
import Split from './src/split'
import Time from './src/time'
import TimeRange from './src/time-range'
import TimeSpinner from './src/time-spinner'
import TransferPanel from './src/transfer-panel'
import UploadList from './src/upload-list'
import Loading from './src/loading'
import Popover from './src/popover'
import Input from './src/input'
import DateRange from './src/date-range'
import Pager from './src/pager'
Expand All @@ -23,16 +34,27 @@ export default {
version,
components: {
Alert,
BreadcrumbItem,
CollapseItem,
Drawer,
Dropdown,
DropdownMenu,
DropdownItem,
FilterBox,
Form,
Guide,
Grid,
Milestone,
Popconfirm,
Popover,
Switch,
Select,
Popover,
Split,
Time,
TimeRange,
TimeSpinner,
TransferPanel,
UploadList,
Loading,
Input,
DateRange,
Expand Down
9 changes: 0 additions & 9 deletions packages/design/aurora/src/alert/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,5 @@ import { iconWarning } from '@opentiny/vue-icon'
export default {
icons: {
warning: iconWarning()
},
renderless: (props, hooks, { emit }, api) => {
const state = api.state
return {
close() {
state.show = false
emit('close')
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
export default {
separator: '/'
separator: '>'
}
7 changes: 7 additions & 0 deletions packages/design/aurora/src/filter-box/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { iconArrowBottom } from '@opentiny/vue-icon'

export default {
icons: {
expandButton: iconArrowBottom()
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider lazy initialization of the icon.

The iconArrowBottom function is being called immediately when the module is imported. This might not be necessary if the icon is not always used. Consider lazy initialization to potentially improve performance.

You could modify the code to use a getter or a function that returns the icon:

import { iconArrowBottom } from '@opentiny/vue-icon'

export default {
  icons: {
    get expandButton() {
      return iconArrowBottom()
    }
  }
}

This way, the icon is only created when it's actually accessed.

}
Comment on lines +4 to +6
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider passing the icon function as a reference.

The current implementation calls iconArrowBottom() immediately. This might not be the intended behavior, as it executes the function at import time rather than when the icon is actually needed.

Consider modifying the code to pass the function reference instead:

 icons: {
-  expandButton: iconArrowBottom()
+  expandButton: iconArrowBottom
 }

This change allows the icon function to be called when it's actually used, which is typically the desired behavior in Vue components.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
icons: {
expandButton: iconArrowBottom()
}
icons: {
expandButton: iconArrowBottom
}

}
26 changes: 26 additions & 0 deletions packages/design/aurora/src/milestone/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
export default {
renderless: (props, hooks, { constants }, api) => {
return {
getMileIcon: (node) => {
const status = props.milestonesStatus[node[props.statusField]] || constants.DEFAULT_COLOR

const isCompleted = node[props.statusField] === props.completedField
const switchColor = isCompleted && !props.solid
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider renaming switchColor for clarity.

The variable switchColor determines whether to swap the background and text colors based on the completion status and the solid prop. A more descriptive name like shouldSwapColors or invertColors could improve code readability.

Apply this diff to rename the variable and update its usage:

 const isCompleted = node[props.statusField] === props.completedField
-const switchColor = isCompleted && !props.solid
+const shouldSwapColors = isCompleted && !props.solid

 return {
-  background: (switchColor ? constants.DEFAULT_BACK_COLOR : status) + '!important',
-  color: (switchColor ? status : constants.DEFAULT_BACK_COLOR) + '!important',
+  background: (shouldSwapColors ? constants.DEFAULT_BACK_COLOR : status) + '!important',
+  color: (shouldSwapColors ? status : constants.DEFAULT_BACK_COLOR) + '!important',
   boxShadow: `rgba(${r},${g},${b},.4) ${constants.BOX_SHADOW_PX}`
 }

Committable suggestion was skipped due to low confidence.

const { r, g, b } = api.hexToRgb(status)
Comment on lines +5 to +9
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential undefined 'status' to prevent runtime errors

In the getMileIcon function, status may be undefined if node[props.statusField] is not a key in props.milestonesStatus and constants.DEFAULT_COLOR is not properly defined. Passing an undefined status to api.hexToRgb(status) could lead to errors.

Consider ensuring that status is always a valid hex color string before using it. You might add validation or default to a safe color value if the retrieved status is invalid.


return {
background: (switchColor ? constants.DEFAULT_BACK_COLOR : status) + '!important',
color: (switchColor ? status : constants.DEFAULT_BACK_COLOR) + '!important',
Comment on lines +12 to +13
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid using !important in inline styles for better CSS practices.

Using !important can make CSS harder to maintain and override, leading to specificity issues. It's generally recommended to adjust the specificity of your selectors or refactor your styles to achieve the desired effect without !important.

Apply this diff to remove !important from the styles:

 return {
   background: (shouldSwapColors ? constants.DEFAULT_BACK_COLOR : status) + '!important',
   color: (shouldSwapColors ? status : constants.DEFAULT_BACK_COLOR) + '!important',
-  background: (shouldSwapColors ? constants.DEFAULT_BACK_COLOR : status) + '!important',
-  color: (shouldSwapColors ? status : constants.DEFAULT_BACK_COLOR) + '!important',
+  background: shouldSwapColors ? constants.DEFAULT_BACK_COLOR : status,
+  color: shouldSwapColors ? status : constants.DEFAULT_BACK_COLOR,
   boxShadow: `rgba(${r},${g},${b},.4) ${constants.BOX_SHADOW_PX}`
 }

Committable suggestion was skipped due to low confidence.

Comment on lines +12 to +13
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid using !important in inline styles for better maintainability

Using !important in inline styles for background and color can make it difficult to override these styles elsewhere and may lead to specificity issues. It's generally best to avoid !important unless absolutely necessary.

Consider refactoring your CSS or styling approach to achieve the desired effect without the need for !important.

boxShadow: `rgba(${r},${g},${b},.4) ${constants.BOX_SHADOW_PX}`
}
},
getFlagStyle: ({ index, idx }) => {
return {
left: `calc(${
(100 / (props.data[props.flagBefore ? index : index + 1][props.flagField].length + 1)) * (idx + 1)
}% - 29px)`
Comment on lines +19 to +21
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify the calculation in getFlagStyle for better readability.

The calculation for the left property is complex and could be difficult to understand or maintain. Breaking it down into intermediate variables can enhance readability.

Apply this diff to refactor the calculation:

 def getFlagStyle: ({ index, idx }) => {
+  const dataIndex = props.flagBefore ? index : index + 1
+  const flagItems = props.data[dataIndex][props.flagField]
+  const itemCount = flagItems.length + 1
+  const positionPercent = (100 / itemCount) * (idx + 1)
+  const leftPosition = `calc(${positionPercent}% - 29px)`
   return {
-    left: `calc(${
-      (100 / (props.data[props.flagBefore ? index : index + 1][props.flagField].length + 1)) * (idx + 1)
-    }% - 29px)`
+    left: leftPosition
   }
 }

Committable suggestion was skipped due to low confidence.


⚠️ Potential issue

Ensure index + 1 does not exceed the bounds of props.data.

In the expression props.data[props.flagBefore ? index : index + 1], if index + 1 exceeds the length of props.data, it could result in an undefined value or an error. Add a check to ensure that index + 1 is within the valid range.

Apply this diff to include a boundary check:

 const getFlagStyle = ({ index, idx }) => {
+  const dataIndexCandidate = props.flagBefore ? index : index + 1
+  const dataIndex = dataIndexCandidate < props.data.length ? dataIndexCandidate : props.data.length - 1
+  const flagItems = props.data[dataIndex][props.flagField]
+  const itemCount = flagItems.length + 1
+  const positionPercent = (100 / itemCount) * (idx + 1)
+  const leftPosition = `calc(${positionPercent}% - 29px)`
   return {
-    left: `calc(${
-      (100 / (props.data[props.flagBefore ? index : index + 1][props.flagField].length + 1)) * (idx + 1)
-    }% - 29px)`
+    left: leftPosition
   }
 }

Committable suggestion was skipped due to low confidence.

Comment on lines +19 to +21
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add checks to prevent potential errors when accessing nested properties

In the getFlagStyle function, the calculation accesses props.data[...]. There is a risk of runtime errors if props.data is undefined, or if the indices index or index + 1 are out of bounds. Similarly, if props.data[...] does not have the props.flagField property or if props.data[...][props.flagField] is not an array, accessing .length could throw an error.

To enhance robustness, consider adding checks to ensure that props.data, props.data[...], and props.data[...][props.flagField] are defined and valid before accessing them.


⚠️ Potential issue

Prevent possible division by zero in left calculation

The calculation for left involves dividing by (props.data[...][props.flagField].length + 1). If props.data[...][props.flagField].length is -1, adding 1 results in 0, leading to a division by zero error.

Ensure that props.data[...][props.flagField].length is not -1, or adjust the calculation to handle such cases to prevent division by zero errors.

}
}
Comment on lines +17 to +23
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Improve clarity and check for potential off-by-one error.

The getFlagStyle method calculates the left position for flags, but the logic is complex and might be difficult to understand or maintain. Additionally, there's a potential off-by-one error in the index calculation when props.flagBefore is true.

Consider refactoring for clarity and correctness:

getFlagStyle: ({ index, idx }) => {
  const flagIndex = props.flagBefore ? index : index + 1;
  const flagCount = props.data[flagIndex][props.flagField].length;
  const position = (idx + 1) / (flagCount + 1);
  const leftPercentage = position * 100;

  return {
    left: `calc(${leftPercentage}% - 29px)`
  }
}

This refactored version:

  1. Clearly separates the index calculation, addressing the potential off-by-one error.
  2. Calculates the position as a fraction, improving readability.
  3. Uses more descriptive variable names.

Please verify that this logic matches the intended behavior, especially regarding the props.flagBefore condition.

}
}
}
7 changes: 7 additions & 0 deletions packages/design/aurora/src/popconfirm/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { iconWarning } from '@opentiny/vue-icon'

export default {
icons: {
warning: iconWarning()
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider lazy initialization of the icon

The warning icon is currently initialized immediately when this module is imported. If the icon is not always used, this could be unnecessary.

Consider lazy initialization:

import { iconWarning } from '@opentiny/vue-icon'

export default {
  icons: {
    get warning() {
      return iconWarning()
    }
  }
}

This approach will only create the icon when it's actually accessed, potentially improving performance.

}
Comment on lines +4 to +6
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider performance and future extensibility of icons.

The current implementation dynamically creates the warning icon. While this works, it might have performance implications if the iconWarning() function is called frequently. Additionally, the current structure might be limiting if more icons need to be added in the future.

Consider the following suggestions:

  1. If the icon doesn't need to be dynamic, create it once and export the result:

    const warningIcon = iconWarning();
    
    export default {
      icons: {
        warning: warningIcon
      }
    };
  2. For better extensibility, consider using a more flexible structure:

    import { iconWarning, iconInfo, iconError } from '@opentiny/vue-icon';
    
    const createIcon = (iconFn: () => any) => iconFn();
    
    export default {
      icons: {
        warning: createIcon(iconWarning),
        info: createIcon(iconInfo),
        error: createIcon(iconError),
        // Easy to add more icons in the future
      }
    };

}
Comment on lines +1 to +7
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider expanding the file content or renaming it

The file name index.ts suggests it might be the main entry point for the popconfirm component, but it only exports icons. Consider either:

  1. Expanding this file to include more popconfirm-related exports, or
  2. Renaming it to something more specific like popconfirm-icons.ts to better reflect its current content.

If you decide to expand the file, you could structure it like this:

import { iconWarning } from '@opentiny/vue-icon'
import { PopconfirmComponent } from './popconfirm-component'

export const PopconfirmIcons = {
  warning: iconWarning
}

export { PopconfirmComponent }

export default {
  component: PopconfirmComponent,
  icons: PopconfirmIcons
}

This structure allows for more flexibility and clear separation of concerns.


🛠️ Refactor suggestion

Add type information for better TypeScript support

To improve type safety and developer experience, consider adding type information to the exported object.

Here's how you could add types:

import { iconWarning } from '@opentiny/vue-icon'

interface PopconfirmIcons {
  warning: ReturnType<typeof iconWarning>
}

const icons: PopconfirmIcons = {
  warning: iconWarning()
}

export default {
  icons
}

This change provides better type inference and documentation for users of this module.

3 changes: 3 additions & 0 deletions packages/design/aurora/src/split/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default {
triggerBarConWithLine: false
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export default {
// 控制time-picker组件时间范围button显示和样式
showTimePickerRangeButton: true
showTimePickerRangeButton: false
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Potential Missing Tests for showTimePickerRangeButton Change

The showTimePickerRangeButton property has been modified from true to false and is referenced in multiple parts of the codebase. However, there are no associated tests to validate this change, which may lead to unintended side effects.

Recommendations:

  • Add Tests: Implement tests to ensure that disabling the time picker range button behaves as expected across all impacted components.
  • Document Rationale: Provide comments or documentation explaining why this change was made to aid future maintenance and understanding.
  • Review Recent Commits: Examine recent changes to ensure this update aligns with overall project goals and does not introduce regressions.
🔗 Analysis chain

Verify the intentionality and impact of disabling the time picker range button.

The showTimePickerRangeButton property has been changed from true to false. This change will hide the time picker range button, which could significantly affect the component's functionality and user experience.

Please confirm:

  1. Is this change intentional?
  2. Have you considered the impact on existing implementations using this component?
  3. Does this change require updates to the component's documentation or usage guidelines?

Additionally, consider adding a comment explaining the rationale behind this change to provide context for future developers.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for usage of showTimePickerRangeButton in the codebase

# Test: Search for references to showTimePickerRangeButton
echo "Searching for references to showTimePickerRangeButton:"
rg --type typescript --type vue 'showTimePickerRangeButton'

# Test: Check if there are any tests related to this property
echo "Checking for related tests:"
rg --type typescript --type vue 'showTimePickerRangeButton' -g '*test*'

# Test: Look for recent changes to files mentioning this property
echo "Checking for recent changes to related files:"
git log -n 5 --pretty=format:"%h - %an, %ar : %s" -- $(rg --files-with-matches 'showTimePickerRangeButton')

Length of output: 1201


Script:

#!/bin/bash
# Description: Check for usage of showTimePickerRangeButton in the codebase

# Test: Search for references to showTimePickerRangeButton in .ts and .vue files
echo "Searching for references to showTimePickerRangeButton in .ts and .vue files:"
rg 'showTimePickerRangeButton' --glob '*.ts' --glob '*.vue'

# Test: Check if there are any tests related to this property
echo "Checking for related tests in .test.ts and .spec.ts files:"
rg 'showTimePickerRangeButton' --glob '*.{test,spec}.ts'

# Test: Look for recent changes to files mentioning this property
echo "Checking for recent changes to related files:"
files=$(rg 'showTimePickerRangeButton' --files-with-matches --glob '*.ts' --glob '*.vue')
if [ -n "$files" ]; then
  git log -n 5 --pretty=format:"%h - %an, %ar : %s" -- $files
else
  echo "No files found with showTimePickerRangeButton references."
fi

Length of output: 2311

Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Potential Breaking Change Detected with showTimePickerRangeButton

The showTimePickerRangeButton property has been changed from true to false, impacting multiple areas of the codebase:

  • packages/vue/src/time-range/src/pc.vue
  • packages/design/saas/src/time-range/index.ts
  • packages/design/aurora/src/time-range/index.ts
  • packages/renderless/src/time-range/vue.ts

This alteration may lead to unintended side effects, such as hiding the range button by default across various components, potentially disrupting the user experience and existing functionalities.

Recommendations:

  • Documentation: Update the component’s documentation to reflect the new default behavior and guide users on how to enable the range button if needed.
  • Migration Strategy: Consider implementing a deprecation period or providing clear migration steps for consumers affected by this change.
  • Backward Compatibility: Evaluate the necessity of maintaining backward compatibility to prevent breaking existing implementations.
🔗 Analysis chain

Please provide context for this change and consider its impact.

The showTimePickerRangeButton property has been changed from true to false, which will hide the time-picker component's range button by default. While this change aligns with the general updates mentioned in the PR description, there are a few points to consider:

  1. Could you please explain the reasoning behind this change? Understanding the motivation will help in evaluating its appropriateness.

  2. This change may impact the user experience and functionality of the time-range component. It's important to ensure that this aligns with the intended design and usability goals.

  3. This could potentially be a breaking change for consumers of this component who rely on the range button being visible by default. Have you considered the backwards compatibility implications?

  4. It would be beneficial to document this change in the component's documentation, explaining the new default behavior and how users can enable the range button if needed.

To assess the impact of this change, let's check for any usage of this property across the codebase:

This will help us understand if there are any components or tests that might be affected by this change.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of showTimePickerRangeButton property
rg --type typescript --type vue 'showTimePickerRangeButton'

Length of output: 94


Script:

#!/bin/bash
# Search for usage of showTimePickerRangeButton property in .ts, .tsx, and .vue files
rg 'showTimePickerRangeButton' --glob '*.{ts,tsx,vue}'

Length of output: 739

}
4 changes: 4 additions & 0 deletions packages/design/aurora/src/time-spinner/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default {
// 时间选择器单个item的margin为0px.
itemMarginSpace: 0
}
4 changes: 4 additions & 0 deletions packages/design/aurora/src/time/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default {
// 控制time-picker组件button显示和样式
showTimePickerButton: false
}
4 changes: 4 additions & 0 deletions packages/design/aurora/src/transfer-panel/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default {
inputBoxType: 'normal',
showInputSearch: false
}
Comment on lines +1 to +4
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding TypeScript interface for better type safety

The exported configuration object is clear and concise. However, to improve type safety and provide better intellisense for consumers of this module, consider defining a TypeScript interface for the configuration object.

Here's a suggested implementation:

interface TransferPanelConfig {
  inputBoxType: 'normal' | string; // Add other possible values if any
  showInputSearch: boolean;
}

const config: TransferPanelConfig = {
  inputBoxType: 'normal',
  showInputSearch: false
};

export default config;

This change will provide better type checking and autocompletion for users of this module.

12 changes: 12 additions & 0 deletions packages/design/aurora/src/upload-list/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export default {
state: {
progressType: 'circle',
progressWidth: null,
progressStrokeWidth: 6,
tooltipDisabled: true
},
icons: {
closeComponent: 'icon-close',
preViewComponent: ''
}
Comment on lines +8 to +11
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Set a value for preViewComponent

The icons object configuration has a potential issue:

  • The closeComponent is correctly set to 'icon-close', which should display the appropriate icon for closing.
  • However, the preViewComponent is set to an empty string ('').

An empty string for preViewComponent might lead to no preview icon being displayed, which could affect the user's ability to preview uploaded files. Consider setting an appropriate value for preViewComponent, such as 'icon-preview' or another suitable icon identifier.

}
30 changes: 26 additions & 4 deletions packages/design/saas/index.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,26 @@
import Alert from './src/alert'
import Badge from './src/badge'
import BreadcrumbItem from './src/breadcrumb-item'
import CollapseItem from './src/collapse-item'
import Drawer from './src/drawer'
import Dropdown from './src/dropdown'
import DropdownMenu from './src/dropdown-menu'
import DropdownItem from './src/dropdown-item'
import FilterBox from './src/filter-box'
import Form from './src/form'
import Guide from './src/guide'
import Grid from './src/grid'
import Milestone from './src/milestone'
import Popconfirm from './src/popconfirm'
import Popover from './src/popover'
import Switch from './src/switch'
import Select from './src/select'
import Popover from './src/popover'
import Split from './src/split'
import Time from './src/time'
import TimeRange from './src/time-range'
import TimeSpinner from './src/time-spinner'
import TransferPanel from './src/transfer-panel'
import UploadList from './src/upload-list'
import Loading from './src/loading'
import Input from './src/input'
import DateRange from './src/date-range'
Expand All @@ -25,22 +36,33 @@ export default {
components: {
Alert,
Badge,
BreadcrumbItem,
CollapseItem,
Drawer,
Dropdown,
DropdownMenu,
DropdownItem,
FilterBox,
Form,
Guide,
Grid,
Milestone,
Pager,
Popconfirm,
Popeditor,
Popover,
Switch,
Select,
Popover,
Split,
Time,
TimeRange,
TimeSpinner,
TransferPanel,
UploadList,
Loading,
Input,
DateRange,
Pager,
DialogBox,
Popeditor,
DatePanel
}
}
9 changes: 0 additions & 9 deletions packages/design/saas/src/alert/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,5 @@ import { iconWarning } from '@opentiny/vue-icon'
export default {
icons: {
warning: iconWarning()
},
renderless: (props, hooks, { emit }, api) => {
const state = api.state
return {
close() {
state.show = false
emit('close')
}
}
}
}
3 changes: 3 additions & 0 deletions packages/design/saas/src/breadcrumb-item/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default {
separator: '>'
}
7 changes: 7 additions & 0 deletions packages/design/saas/src/filter-box/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { iconArrowBottom } from '@opentiny/vue-icon'

export default {
icons: {
expandButton: iconArrowBottom()
}
}
Comment on lines +3 to +7
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider lazy loading the icon for better performance.

The current implementation immediately calls iconArrowBottom() when the module is imported. For better performance, especially if the icon is not always used, consider lazy loading the icon:

 export default {
   icons: {
-    expandButton: iconArrowBottom()
+    expandButton: () => iconArrowBottom()
   }
 }

This change would defer the icon creation until it's actually needed, potentially improving initial load time.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export default {
icons: {
expandButton: iconArrowBottom()
}
}
export default {
icons: {
expandButton: () => iconArrowBottom()
}
}

Comment on lines +3 to +7
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding type information and lazy evaluation.

The exported object structure is clear, but there are two suggestions for improvement:

  1. Add type information for better maintainability and developer experience.
  2. Consider lazy evaluation of the icon function to potentially improve performance.

Here's a suggested implementation with these improvements:

import { IconComponent } from '@opentiny/vue-icon'
import { iconArrowBottom } from '@opentiny/vue-icon'

interface FilterBoxConfig {
  icons: {
    expandButton: () => IconComponent
  }
}

export default {
  icons: {
    expandButton: () => iconArrowBottom()
  }
} as FilterBoxConfig

This implementation:

  • Adds type information for better maintainability.
  • Uses lazy evaluation for the icon function, which may improve performance by only creating the icon when it's actually used.

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
export default {
state: {
btnOrderReversed: true
isUseModalOverlay: true
}
}
26 changes: 26 additions & 0 deletions packages/design/saas/src/milestone/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
export default {
renderless: (props, hooks, { constants }, api) => {
return {
getMileIcon: (node) => {
const status = props.milestonesStatus[node[props.statusField]] || constants.DEFAULT_COLOR
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle undefined or null values in node[props.statusField]

Accessing node[props.statusField] without checking may lead to errors if the value is undefined or null. Ensure that this value is defined before using it as a key in props.milestonesStatus.

Consider adding a default value or a check:

- const status = props.milestonesStatus[node[props.statusField]] || constants.DEFAULT_COLOR
+ const statusKey = node[props.statusField] || 'defaultStatus';
+ const status = props.milestonesStatus[statusKey] || constants.DEFAULT_COLOR
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const status = props.milestonesStatus[node[props.statusField]] || constants.DEFAULT_COLOR
const statusKey = node[props.statusField] || 'defaultStatus';
const status = props.milestonesStatus[statusKey] || constants.DEFAULT_COLOR


const isCompleted = node[props.statusField] === props.completedField
const switchColor = isCompleted && !props.solid
const { r, g, b } = api.hexToRgb(status)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate color code before converting with api.hexToRgb

If status is not a valid hex color code, api.hexToRgb(status) may throw an error. Ensure that status is a valid hex color code before conversion or handle potential exceptions.

Consider adding validation:

- const { r, g, b } = api.hexToRgb(status)
+ let r, g, b;
+ if (api.isValidHex(status)) {
+   ({ r, g, b } = api.hexToRgb(status));
+ } else {
+   // Handle invalid color code, perhaps assign default RGB values
+   ({ r, g, b } = { r: 0, g: 0, b: 0 });
+ }

Committable suggestion was skipped due to low confidence.

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential invalid color values in api.hexToRgb(status)

The function api.hexToRgb(status) assumes that status is always a valid hex color string. If status is undefined or an invalid color code, this could lead to runtime errors.

Consider adding validation to ensure status is a valid hex color before attempting to convert it, or handle potential errors within the hexToRgb function to improve robustness.


return {
background: (switchColor ? constants.DEFAULT_BACK_COLOR : status) + '!important',
color: (switchColor ? status : constants.DEFAULT_BACK_COLOR) + '!important',
Comment on lines +12 to +13
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid using !important in inline styles

Using !important in inline styles can make it difficult to override styles elsewhere and may lead to maintenance challenges. Consider refactoring to eliminate the need for !important.

Apply this diff to remove !important from the styles:

- background: (switchColor ? constants.DEFAULT_BACK_COLOR : status) + '!important',
- color: (switchColor ? status : constants.DEFAULT_BACK_COLOR) + '!important',
+ background: switchColor ? constants.DEFAULT_BACK_COLOR : status,
+ color: switchColor ? status : constants.DEFAULT_BACK_COLOR,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
background: (switchColor ? constants.DEFAULT_BACK_COLOR : status) + '!important',
color: (switchColor ? status : constants.DEFAULT_BACK_COLOR) + '!important',
background: switchColor ? constants.DEFAULT_BACK_COLOR : status,
color: switchColor ? status : constants.DEFAULT_BACK_COLOR,

boxShadow: `rgba(${r},${g},${b},.4) ${constants.BOX_SHADOW_PX}`
}
},
Comment on lines +4 to +16
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance getMileIcon method for better readability, performance, and error handling.

While the getMileIcon method's logic appears correct, there are several areas where it could be improved:

  1. Readability: Consider extracting some of the logic into smaller, named functions.
  2. Performance: The hexToRgb conversion could potentially be memoized if the same colors are used frequently.
  3. Error Handling: Add checks for invalid color values or missing properties.
  4. Avoid !important: If possible, restructure your CSS to avoid using !important.

Here's a suggested refactor:

const getMileIcon = (node) => {
  const getStatus = (node) => props.milestonesStatus[node[props.statusField]] || constants.DEFAULT_COLOR;
  const isCompleted = (node) => node[props.statusField] === props.completedField;
  const getSwitchColor = (node) => isCompleted(node) && !props.solid;

  const status = getStatus(node);
  const switchColor = getSwitchColor(node);

  let backgroundColor, textColor;
  try {
    const { r, g, b } = api.hexToRgb(status);
    backgroundColor = switchColor ? constants.DEFAULT_BACK_COLOR : status;
    textColor = switchColor ? status : constants.DEFAULT_BACK_COLOR;

    return {
      background: `${backgroundColor}`,
      color: `${textColor}`,
      boxShadow: `rgba(${r},${g},${b},.4) ${constants.BOX_SHADOW_PX}`
    };
  } catch (error) {
    console.error('Error processing colors:', error);
    return {}; // or some default styles
  }
};

This refactored version improves readability, adds error handling, and removes the !important flags. Consider memoizing the hexToRgb function if performance becomes an issue with many color conversions.

getFlagStyle: ({ index, idx }) => {
return {
left: `calc(${
(100 / (props.data[props.flagBefore ? index : index + 1][props.flagField].length + 1)) * (idx + 1)
}% - 29px)`
Comment on lines +19 to +21
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure safe access to array elements in getFlagStyle

The calculation in getFlagStyle uses props.data[props.flagBefore ? index : index + 1], which could result in an index out-of-bounds error if index + 1 exceeds the array length. Ensure that index + 1 is within bounds to prevent runtime errors.

Consider adding a boundary check before accessing the array:

+ const dataIndex = props.flagBefore ? index : index + 1;
+ if (dataIndex >= props.data.length) {
+   // Handle out-of-bounds index appropriately
+ }
  left: `calc(${
-   (100 / (props.data[props.flagBefore ? index : index + 1][props.flagField].length + 1)) * (idx + 1)
+   (100 / (props.data[dataIndex][props.flagField].length + 1)) * (idx + 1)
  }% - 29px)`

Committable suggestion was skipped due to low confidence.

Comment on lines +19 to +21
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure safe access to nested properties in props.data

The calculation for the left property accesses props.data[props.flagBefore ? index : index + 1][props.flagField].length. If props.data does not have the expected structure, or if any nested property is undefined, this could cause runtime errors.

Add checks to confirm that props.data, props.data[...], and props.data[...][props.flagField] exist before accessing length. This will prevent potential exceptions due to undefined properties.

}
}
Comment on lines +17 to +23
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve getFlagStyle method for better readability and error handling.

The getFlagStyle method could benefit from some improvements:

  1. Readability: The calculation is complex and could be broken down into smaller parts.
  2. Error Handling: Add checks for potential division by zero or missing properties.
  3. Magic Numbers: The -29px value should be explained or moved to a named constant.

Here's a suggested refactor:

const FLAG_OFFSET = -29; // px, explain why this value is used

const getFlagStyle = ({ index, idx }) => {
  const getFlagCount = (index) => {
    const dataIndex = props.flagBefore ? index : index + 1;
    return props.data[dataIndex]?.[props.flagField]?.length ?? 0;
  };

  const calculateLeftPosition = (flagCount, idx) => {
    if (flagCount === 0) return 0;
    const stepPercentage = 100 / (flagCount + 1);
    return stepPercentage * (idx + 1);
  };

  const flagCount = getFlagCount(index);
  const leftPercentage = calculateLeftPosition(flagCount, idx);

  return {
    left: `calc(${leftPercentage}% ${FLAG_OFFSET}px)`
  };
};

This refactored version improves readability by breaking down the calculation into smaller, named functions. It also adds some basic error handling for missing data or properties. The magic number is now a named constant, which should be accompanied by a comment explaining its purpose.

}
}
}
Comment on lines +1 to +26
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding TypeScript type annotations to improve code clarity and maintainability.

While the overall structure of the renderless component looks good, adding TypeScript type annotations would greatly improve the code's clarity and help catch potential errors early. Consider defining interfaces for the props, hooks, constants, and api parameters, as well as for the return types of getMileIcon and getFlagStyle.

Here's an example of how you could start adding type annotations:

interface Props {
  milestonesStatus: Record<string, string>;
  statusField: string;
  completedField: string;
  solid: boolean;
  data: any[]; // Replace 'any' with a more specific type
  flagField: string;
  flagBefore: boolean;
}

interface Constants {
  DEFAULT_COLOR: string;
  DEFAULT_BACK_COLOR: string;
  BOX_SHADOW_PX: string;
}

interface Api {
  hexToRgb: (hex: string) => { r: number; g: number; b: number };
}

export default {
  renderless: (props: Props, hooks: any, { constants }: { constants: Constants }, api: Api) => {
    // ... rest of the code
  }
};

This will provide better type safety and make the code more self-documenting.

7 changes: 7 additions & 0 deletions packages/design/saas/src/popconfirm/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { iconWarning } from '@opentiny/vue-icon'

export default {
icons: {
warning: iconWarning()
}
Comment on lines +4 to +6
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Add type information to the icons object.

The icons object in packages/design/saas/src/popconfirm/index.ts does not have a type annotation. Adding type information will enhance code clarity and maintainability.

🔗 Analysis chain

Consider adding type information and reviewing icon requirements.

The initialization of the warning icon looks good. However, there are a couple of suggestions for improvement:

  1. Add type information to enhance code clarity and maintainability. For example:

    interface Icons {
      warning: ReturnType<typeof iconWarning>;
    }
    
    const icons: Icons = {
      warning: iconWarning()
    };
  2. Review if the popconfirm component might need additional icons (e.g., success, error) to cover all use cases.

To verify if other icons are used in the popconfirm component, you can run:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for icon usage in popconfirm-related files
rg --type typescript --type vue 'icon' packages/design/saas/src/popconfirm

Length of output: 109


Script:

#!/bin/bash
# Search for icon usage in popconfirm-related files with correct type identifiers
rg --type ts --type vue 'icon' packages/design/saas/src/popconfirm

Length of output: 94


Script:

#!/bin/bash
# Search for icon usage in popconfirm-related files with correct type identifiers
rg --type ts --type-add 'vue:*.vue' --type vue 'icon' packages/design/saas/src/popconfirm

Length of output: 311


Script:

#!/bin/bash
# Check if 'icons' object has a type annotation in index.ts
rg 'icons\s*:\s*\w+' packages/design/saas/src/popconfirm/index.ts

Length of output: 67

}
Comment on lines +3 to +7
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider lazy initialization and add type information.

The current implementation initializes the warning icon immediately when the module is imported. Consider the following suggestions:

  1. Lazy initialization: This could improve performance if the icon isn't always used.
  2. Add TypeScript type information: This would enhance type safety and developer experience.

Here's a suggested implementation incorporating these changes:

import { iconWarning } from '@opentiny/vue-icon'

interface Icons {
  warning: ReturnType<typeof iconWarning>
}

interface PopconfirmConfig {
  icons: {
    warning: () => Icons['warning']
  }
}

const config: PopconfirmConfig = {
  icons: {
    warning: () => iconWarning()
  }
}

export default config

This approach lazily initializes the icon and provides type information. The warning property is now a function that returns the icon, ensuring it's only created when needed.

3 changes: 3 additions & 0 deletions packages/design/saas/src/split/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default {
triggerBarConWithLine: false
}
4 changes: 4 additions & 0 deletions packages/design/saas/src/time-range/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default {
// 控制time-picker组件时间范围button显示和样式
showTimePickerRangeButton: false
}
4 changes: 4 additions & 0 deletions packages/design/saas/src/time-spinner/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default {
// 时间选择器单个item的margin为0px.
itemMarginSpace: 0
}
4 changes: 4 additions & 0 deletions packages/design/saas/src/time/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default {
// 控制time-picker组件button显示和样式
showTimePickerButton: false
}
4 changes: 4 additions & 0 deletions packages/design/saas/src/transfer-panel/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default {
inputBoxType: 'normal',
showInputSearch: false
}
12 changes: 12 additions & 0 deletions packages/design/saas/src/upload-list/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export default {
state: {
progressType: 'circle',
progressWidth: null,
progressStrokeWidth: 6,
tooltipDisabled: true
},
icons: {
closeComponent: 'icon-close',
preViewComponent: ''
}
Comment on lines +8 to +11
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Potential Issue: preViewComponent is Unused

The icons object includes preViewComponent set to an empty string. However, no usage was found in .vue, .ts, or .js files within the packages/design/saas/src directory.

  • Consider removing preViewComponent if it's not needed.
  • If it's intended for future use, add a TODO comment to clarify its purpose.
🔗 Analysis chain

LGTM! Clarification needed for preViewComponent.

The icons object is structured well. The closeComponent is clearly defined. However, the preViewComponent is set to an empty string.

Could you clarify if this is intentional or if it's meant to be implemented later? If it's a placeholder, consider adding a TODO comment for future implementation.

To verify the usage of these components, let's run the following script:

This will help us understand if these components are being used elsewhere in the codebase and if preViewComponent is referenced despite being empty.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for usage of closeComponent and preViewComponent

# Search for closeComponent usage
echo "Searching for closeComponent usage:"
rg --type vue "closeComponent" packages/design/saas/src

# Search for preViewComponent usage
echo "Searching for preViewComponent usage:"
rg --type vue "preViewComponent" packages/design/saas/src

Length of output: 335


Script:

#!/bin/bash
# Description: Check for usage of closeComponent and preViewComponent in .vue files

# Search for closeComponent usage
echo "Searching for closeComponent usage in .vue files:"
rg "closeComponent" packages/design/saas/src -g "*.vue"

# Search for preViewComponent usage
echo "Searching for preViewComponent usage in .vue files:"
rg "preViewComponent" packages/design/saas/src -g "*.vue"

Length of output: 335


Script:

#!/bin/bash
# Description: Check for usage of closeComponent and preViewComponent in relevant file types

# Search for closeComponent usage in .vue, .ts, and .js files
echo "Searching for closeComponent usage in .vue, .ts, and .js files:"
rg "closeComponent" packages/design/saas/src -g "*.vue" -g "*.ts" -g "*.js"

# Search for preViewComponent usage in .vue, .ts, and .js files
echo "Searching for preViewComponent usage in .vue, .ts, and .js files:"
rg "preViewComponent" packages/design/saas/src -g "*.vue" -g "*.ts" -g "*.js"

Length of output: 582

}
Comment on lines +1 to +12
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing type safety, documentation, and file naming

While the overall structure of this configuration object is clean and well-organized, there are a few general improvements to consider:

  1. Implement TypeScript interfaces or types for the entire configuration object to enhance type safety and provide better autocomplete support for consumers of this module.

  2. Add JSDoc comments to describe the purpose of this configuration object and how it should be used in the context of the upload list component.

  3. Consider renaming the file to something more specific, such as upload-list-config.ts or upload-list-defaults.ts, to better reflect its contents and purpose.

  4. If this configuration is meant to be customizable, consider adding a brief example in the file comments showing how users can override these default settings.

Example:

/**
 * Default configuration for the UploadList component.
 * 
 * @example
 * import uploadListConfig from './upload-list-config';
 * 
 * const customConfig = {
 *   ...uploadListConfig,
 *   state: {
 *     ...uploadListConfig.state,
 *     progressType: 'line',
 *   },
 * };
 */

interface UploadListConfig {
  state: {
    progressType: 'circle' | 'line';
    progressWidth: number | null;
    progressStrokeWidth: number;
    tooltipDisabled: boolean;
  };
  icons: {
    closeIconName: string;
    previewIconName: string | null;
  };
}

const uploadListConfig: UploadListConfig = {
  // ... (your existing configuration)
};

export default uploadListConfig;

These changes would significantly improve the maintainability and usability of this configuration module.

39 changes: 1 addition & 38 deletions packages/design/smb/index.ts
Original file line number Diff line number Diff line change
@@ -1,44 +1,7 @@
import Alert from './src/alert'
import ActionMenu from './src/action-menu'
import Popconfirm from './src/popconfirm'
import Drawer from './src/drawer'
import Dropdown from './src/dropdown'
import DropdownItem from './src/dropdown-item'
import FilterBox from './src/filter-box'
import Guide from './src/guide'
import Select from './src/select'
import TreeNode from './src/tree-node'
import TimeSpinner from './src/time-spinner'
import TimeRange from './src/time-range'
import Time from './src/time-spinner'
import UploadList from './src/upload-list'
import BreadcrumbItem from './src/breadcrumb-item'
import Milestone from './src/milestone'
import Split from './src/split'
import TransferPanel from './src/transfer-panel'
import { version } from './package.json'

export default {
name: 'smb',
version,
components: {
Alert,
ActionMenu,
Popconfirm,
Drawer,
Dropdown,
DropdownItem,
FilterBox,
Guide,
Select,
TreeNode,
TimeSpinner,
TimeRange,
Time,
BreadcrumbItem,
UploadList,
Milestone,
Split,
TransferPanel
}
components: {}
}
5 changes: 0 additions & 5 deletions packages/design/smb/src/action-menu/index.ts

This file was deleted.

16 changes: 0 additions & 16 deletions packages/design/smb/src/alert/index.ts

This file was deleted.

Loading
Loading