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: [color-select-panel] color select panel #2529

Merged
merged 15 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion examples/sites/demos/pc/app/color-select-panel/base.vue
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<div>
<tiny-button @click="changeVisible">Show Color select panel</tiny-button>
<div style="position: relative">
<tiny-color-select-panel v-model="color" :visible="visible" @confirm="hidden" @cancel="hidden" />
<tiny-color-select-panel v-model="color" :visible="visible" @confirm="onConfirm" @cancel="hidden" />
</div>
</div>
</template>
Expand All @@ -25,6 +25,10 @@ export default {
changeVisible() {
this.visible = !this.visible
},
onConfirm(color) {
console.log(color)
this.hidden()
},
hidden() {
this.visible = false
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<template>
<div>
<tiny-button @click="changeVisible">Show Color select panel</tiny-button>
<div style="position: relative">
<tiny-color-select-panel
v-model="color"
:visible="visible"
@color-update="onColorUpdate"
@confirm="onConfirm"
@cancel="hidden"
/>
</div>
</div>
</template>

<script setup>
import { TinyColorSelectPanel, TinyButton, TinyNotify } from '@opentiny/vue'
import { ref } from 'vue'

const color = ref('#66ccff')
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add color value validation

Consider adding validation for the color value to ensure it's a valid hex color code.

-const color = ref('#66ccff')
+const isValidHexColor = (color: string) => /^#[0-9A-Fa-f]{6}$/.test(color)
+const color = ref('#66ccff')
+watch(color, (newValue) => {
+  if (!isValidHexColor(newValue)) {
+    console.warn('Invalid hex color code:', newValue)
+    color.value = '#66ccff' // Reset to default
+  }
+})
📝 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 color = ref('#66ccff')
const isValidHexColor = (color: string) => /^#[0-9A-Fa-f]{6}$/.test(color)
const color = ref('#66ccff')
watch(color, (newValue) => {
if (!isValidHexColor(newValue)) {
console.warn('Invalid hex color code:', newValue)
color.value = '#66ccff' // Reset to default
}
})

const visible = ref(false)
const changeVisible = () => {
visible.value = !visible.value
}
const hidden = () => {
visible.value = false
}
const onConfirm = () => {
hidden()
}
const onColorUpdate = () => {
TinyNotify({
type: 'success',
position: 'top-right',
title: '用户更改了颜色'
})
}
Comment on lines +31 to +37
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 color update notification

The current implementation has several issues:

  1. The notification message is hardcoded in Chinese
  2. It triggers on every color update which might be too frequent
  3. No debouncing is implemented
+import { debounce } from 'lodash-es'
+
-const onColorUpdate = () => {
+const onColorUpdate = debounce(() => {
   TinyNotify({
     type: 'success',
     position: 'top-right',
-    title: '用户更改了颜色'
+    title: 'Color updated'  // Consider using i18n for translations
   })
-}
+}, 300)  // Debounce for 300ms
📝 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 onColorUpdate = () => {
TinyNotify({
type: 'success',
position: 'top-right',
title: '用户更改了颜色'
})
}
import { debounce } from 'lodash-es'
const onColorUpdate = debounce(() => {
TinyNotify({
type: 'success',
position: 'top-right',
title: 'Color updated' // Consider using i18n for translations
})
}, 300) // Debounce for 300ms

</script>
49 changes: 49 additions & 0 deletions examples/sites/demos/pc/app/color-select-panel/color-update.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<template>
<div>
<tiny-button @click="changeVisible">Show Color select panel</tiny-button>
<div style="position: relative">
<tiny-color-select-panel
v-model="color"
:visible="visible"
@color-update="onColorUpdate"
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Event is properly implemented but lacks detailed documentation

The color-update event is properly implemented in the component:

  • It's correctly declared in the component's emits array
  • It's emitted when the color changes in the renderless logic
  • It's demonstrated in demo files

However, there's no detailed API documentation found for this event. Consider adding documentation that explains:

  • When the event is triggered
  • What payload it provides (the new color value)
  • Its intended use cases
🔗 Analysis chain

Verify color-update event documentation

The component uses the color-update event, but we should ensure this is properly documented in the component's documentation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for color-update event documentation
rg -t md -t vue "color-update.*event" docs/

Length of output: 73


Script:

#!/bin/bash
# Search for color-update event documentation with correct file type and broader scope
rg "color-update.*event|@color-update" -g "*.{md,vue}"

Length of output: 429


Script:

#!/bin/bash
# Search for the component that emits this event
ast-grep --pattern 'emit($_, "color-update")'

Length of output: 47


Script:

#!/bin/bash
# Look for any documentation about color events
rg "color.*event" -g "*.{md,vue}"

Length of output: 1064


Script:

#!/bin/bash
# Let's check the component implementation to find the emit
rg "color-update" -g "*.{js,ts,vue}" -A 5 -B 5 packages/

Length of output: 2728


Script:

#!/bin/bash
# Check the demo files to understand the usage context
cat examples/sites/demos/pc/app/color-select-panel/color-update.vue

Length of output: 989

@confirm="onConfirm"
@cancel="hidden"
/>
</div>
</div>
</template>

<script>
import { TinyColorSelectPanel, TinyButton, TinyNotify } from '@opentiny/vue'

export default {
components: {
TinyColorSelectPanel,
TinyButton
},
data() {
return {
color: '#66ccff',
visible: false
}
},
methods: {
changeVisible() {
this.visible = !this.visible
},
onConfirm() {
this.hidden()
},
hidden() {
this.visible = false
},
onColorUpdate() {
TinyNotify({
type: 'success',
position: 'top-right',
title: '用户更改了颜色'
})
}
Comment on lines +40 to +46
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 notification handling and add internationalization support

The current implementation has several areas for improvement:

  1. The notification message is hardcoded in Chinese
  2. Missing error handling for color updates
  3. Notification configuration is hardcoded

Consider applying these changes:

+ const NOTIFICATION_CONFIG = {
+   type: 'success',
+   position: 'top-right'
+ }
+
 export default {
   // ... other code ...
   methods: {
     onColorUpdate() {
+      try {
         TinyNotify({
-          type: 'success',
-          position: 'top-right',
-          title: '用户更改了颜色'
+          ...NOTIFICATION_CONFIG,
+          title: this.$t('colorSelect.colorChanged') // Add translation key
         })
+      } catch (error) {
+        TinyNotify({
+          type: 'error',
+          ...NOTIFICATION_CONFIG,
+          title: this.$t('colorSelect.updateError')
+        })
+      }
     }
   }
 }

Committable suggestion skipped: line range outside the PR's diff.

}
}
</script>
3 changes: 1 addition & 2 deletions examples/sites/demos/pc/app/color-select-panel/event.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,5 @@ test('事件触发', async ({ page }) => {
const demo = await page.locator('#event')
await demo.getByRole('button', { name: 'Show Color select panel' }).click()
await demo.getByRole('button', { name: '取消' }).click()
expect(page.locator('.tiny-notify')).toHaveText('用户点击了取消')
await page.locator('.tiny-notify__icon-close > .tiny-svg').click()
expect(page.getByText('用户点击了取消').first()).not.toBeNull()
})
Original file line number Diff line number Diff line change
Expand Up @@ -17,27 +17,16 @@

<script setup>
import { ref } from 'vue'
import { TinyColorSelectPanel, TinyButton, TinyNotify } from '@opentiny/vue'
import { TinyColorSelectPanel, TinyButton } from '@opentiny/vue'

const color = ref('#66ccff')
const visible = ref(false)
const changeVisible = () => (visible.value = !visible.value)
const hidden = () => (visible.value = false)
const onConfirm = (msg) => {
TinyNotify({
type: 'success',
position: 'top-right',
title: '用户点击了选择',
message: msg
})
hidden()
}
const onCancel = () => {
TinyNotify({
type: 'error',
position: 'top-right',
title: '用户点击了取消'
})
hidden()
}
const history = ref(['#66ccff'])
Expand Down
13 changes: 1 addition & 12 deletions examples/sites/demos/pc/app/color-select-panel/history.vue
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
</template>

<script>
import { TinyColorSelectPanel, TinyButton, TinyNotify } from '@opentiny/vue'
import { TinyColorSelectPanel, TinyButton } from '@opentiny/vue'

export default {
components: {
Expand All @@ -39,20 +39,9 @@ export default {
this.visible = false
},
onConfirm(msg) {
TinyNotify({
type: 'success',
position: 'top-right',
title: '用户点击了选择',
message: msg
})
this.hidden()
},
onCancel() {
TinyNotify({
type: 'error',
position: 'top-right',
title: '用户点击了取消'
})
this.hidden()
},
randomHex() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,16 @@

<script setup>
import { ref } from 'vue'
import { TinyColorSelectPanel, TinyButton, TinyNotify } from '@opentiny/vue'
import { TinyColorSelectPanel, TinyButton } from '@opentiny/vue'

const color = ref('#66ccff')
const visible = ref(false)
const changeVisible = () => (visible.value = !visible.value)
const hidden = () => (visible.value = false)
const onConfirm = (msg) => {
TinyNotify({
type: 'success',
position: 'top-right',
title: '用户点击了选择',
message: msg
})
const onConfirm = () => {
hidden()
}
const onCancel = () => {
TinyNotify({
type: 'error',
position: 'top-right',
title: '用户点击了取消'
})
hidden()
}
const randomHex = () =>
Expand Down
14 changes: 2 additions & 12 deletions examples/sites/demos/pc/app/color-select-panel/predefine.vue
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<template>
<div>
<p>{{ color }}</p>
<tiny-button @click="changeVisible">Show Color select panel</tiny-button>
<tiny-button @click="addPredefineColor">Append predefine color</tiny-button>
<tiny-button @click="popPredefineColor">Pop predefine color</tiny-button>
Expand All @@ -17,7 +18,7 @@
</template>

<script>
import { TinyColorSelectPanel, TinyButton, TinyNotify } from '@opentiny/vue'
import { TinyColorSelectPanel, TinyButton } from '@opentiny/vue'

export default {
components: {
Expand All @@ -39,20 +40,9 @@ export default {
this.visible = false
},
onConfirm(msg) {
TinyNotify({
type: 'success',
position: 'top-right',
title: '用户点击了选择',
message: msg
})
this.hidden()
},
onCancel() {
TinyNotify({
type: 'error',
position: 'top-right',
title: '用户点击了取消'
})
this.hidden()
},
randomHex() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,20 @@ export default {
'By<code>predefine</code>predefined color values, user behavior does not change the predefined colors, but they can be changed externally.'
},
codeFiles: ['predefine.vue']
},
{
demoId: 'colorUpdate',
name: {
'zh-CN': '颜色修改事件',
'en-US': 'Color update event'
},
desc: {
'zh-CN':
'通过<code>@color-update</code>来监听颜色修改事件, 注意: 只在颜色修改时会触发改事件, 例如拖拽光标或修改色相、透明度时',
'en-US':
'Listen for color modification events through<code>@color-update</code>. Note that the change event will only be triggered when the color is modified, such as when dragging the cursor or changing the hue or transparency'
},
codeFiles: ['color-update.vue']
}
]
}
91 changes: 75 additions & 16 deletions packages/renderless/src/color-select-panel/alpha-select/index.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,82 @@
import type { IColorSelectPanelRef } from '@/types'
import type { IColorSelectPanelAlphProps, IColorSelectPanelRef, ISharedRenderlessParamHooks } from '@/types'
import type { Color } from '../utils/color'
import { getClientXY } from '../utils/getClientXY'

export const calcLeftByAlpha = (wrapper: HTMLElement, thumb: HTMLElement, alpha: number) => {
return Math.round((alpha * (wrapper.offsetWidth - thumb.offsetWidth / 2)) / 100)
type State = ReturnType<typeof initState>

export const initState = ({ ref, reactive }: ISharedRenderlessParamHooks) => {
const background = ref('')
const left = ref(0)
const state = reactive({ background, left })
return state
}

export const updateThumb = (alpha: number, thumb: HTMLElement, wrapper: HTMLElement) => {
thumb.style.left = `${calcLeftByAlpha(wrapper, thumb, alpha)}px`
export const useEvent = (
state: State,
slider: IColorSelectPanelRef<HTMLElement | undefined>,
alphaWrapper: IColorSelectPanelRef<HTMLElement | undefined>,
alphaThumb: IColorSelectPanelRef<HTMLElement | undefined>,
props: IColorSelectPanelAlphProps<Color>
) => {
const onDrag = (event: MouseEvent | TouchEvent) => {
if (!slider.value || !alphaThumb.value) {
return
}
const el = alphaWrapper.value!
const rect = el.getBoundingClientRect()
const { clientX } = getClientXY(event)
let left = clientX - rect.left
left = Math.max(alphaThumb.value.offsetWidth / 2, left)
left = Math.min(left, rect.width - alphaThumb.value.offsetWidth / 2)
const alpha = Math.round(
((left - alphaThumb.value.offsetWidth / 2) / (rect.width - alphaThumb.value.offsetWidth)) * 100
)
props.color.set('alpha', alpha)
}
const onClick = (event: MouseEvent | TouchEvent) => {
Comment on lines +21 to +35
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 alphaWrapper.value is checked before use

In the onDrag function, alphaWrapper.value is accessed without a null check. This could lead to runtime errors if alphaWrapper.value is null or undefined.

Consider adding alphaWrapper.value to the existing null checks:

 const onDrag = (event: MouseEvent | TouchEvent) => {
-  if (!slider.value || !alphaThumb.value) {
+  if (!slider.value || !alphaThumb.value || !alphaWrapper.value) {
     return
   }
-  const el = alphaWrapper.value!
+  const el = alphaWrapper.value
   const rect = el.getBoundingClientRect()
   // rest of the code...
 }
📝 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 onDrag = (event: MouseEvent | TouchEvent) => {
if (!slider.value || !alphaThumb.value) {
return
}
const el = alphaWrapper.value!
const rect = el.getBoundingClientRect()
const { clientX } = getClientXY(event)
let left = clientX - rect.left
left = Math.max(alphaThumb.value.offsetWidth / 2, left)
left = Math.min(left, rect.width - alphaThumb.value.offsetWidth / 2)
const alpha = Math.round(
((left - alphaThumb.value.offsetWidth / 2) / (rect.width - alphaThumb.value.offsetWidth)) * 100
)
props.color.set('alpha', alpha)
}
const onDrag = (event: MouseEvent | TouchEvent) => {
if (!slider.value || !alphaThumb.value || !alphaWrapper.value) {
return
}
const el = alphaWrapper.value
const rect = el.getBoundingClientRect()
const { clientX } = getClientXY(event)
let left = clientX - rect.left
left = Math.max(alphaThumb.value.offsetWidth / 2, left)
left = Math.min(left, rect.width - alphaThumb.value.offsetWidth / 2)
const alpha = Math.round(
((left - alphaThumb.value.offsetWidth / 2) / (rect.width - alphaThumb.value.offsetWidth)) * 100
)
props.color.set('alpha', alpha)
}

if (event.target !== alphaThumb.value) {
onDrag(event)
}
}
const getLeft = () => {
const thumb = alphaThumb.value
if (!thumb) {
return 0
}
const el = alphaWrapper.value
if (!el) {
return 0
}
const alpha = props.color.get('alpha')
return (alpha * (el.offsetWidth - thumb.offsetWidth / 2)) / 100
}
const getBackground = () => {
if (props.color && props.color.value) {
const { r, g, b } = props.color.toRgb()
return `linear-gradient(to right, rgba(${r}, ${g}, ${b}, 0) 0%, rgba(${r}, ${g}, ${b}, 1) 100%)`
}
return ''
}
const update = () => {
state.left = getLeft()
state.background = getBackground()
}
return { onDrag, onClick, update }
}

export const onDrag = (
event: MouseEvent,
bar: IColorSelectPanelRef<HTMLElement>,
thumb: IColorSelectPanelRef<HTMLElement>,
alpha: IColorSelectPanelRef<number>
export const initWatch = (
props: IColorSelectPanelAlphProps<Color>,
update: () => void,
{ watch }: ISharedRenderlessParamHooks
) => {
const rect = bar.value.getBoundingClientRect()
const { clientX } = event
let left = clientX - rect.left
left = Math.max(thumb.value.offsetWidth / 2, left)
left = Math.min(left, rect.width - thumb.value.offsetWidth / 2)
alpha.value = Math.round(((left - thumb.value.offsetWidth / 2) / (rect.width - thumb.value.offsetWidth)) * 100)
watch(
() => props.color.get('alpha'),
() => update(),
{ deep: true }
)
watch(
() => props.color,
() => update(),
{ deep: true }
)
}
Loading
Loading