Don't render swatches in color picker for empty array #1116
Conversation
|
Hi there @bjarnef, thank you for this contribution! 👍 While we wait for the team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where swatches are rendered in the color picker even when an empty array is provided. The changes ensure consistency with the swatches component by adding a check for empty arrays and updating associated story files.
- Updated UUIColorPickerElement to return nothing when swatches is empty.
- Added a NoSwatches story in both the color swatches and color picker story files.
- Introduced a TransparentSwatches story in the color picker story file.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/uui-color-swatches/lib/uui-color-swatches.story.ts | Added a NoSwatches story to test empty swatches behavior. |
| packages/uui-color-picker/lib/uui-color-picker.story.ts | Added NoSwatches and TransparentSwatches stories with test data for swatches and opacity. |
| packages/uui-color-picker/lib/uui-color-picker.element.ts | Updated _renderSwatches to return nothing if swatches array is empty. |
| @@ -530,7 +530,7 @@ export class UUIColorPickerElement extends LabelMixin('label', LitElement) { | |||
| } | |||
|
|
|||
| private _renderSwatches() { | |||
| if (!this.swatches) return nothing; | |||
| if (!this.swatches || this.swatches.length === 0) return nothing; | |||
There was a problem hiding this comment.
[nitpick] Consider using optional chaining for a more concise empty check, for example: 'if (!this.swatches?.length) return nothing;'.
| if (!this.swatches || this.swatches.length === 0) return nothing; | |
| if (!this.swatches?.length) return nothing; |
Copilot uses AI. Check for mistakes.
|
|
@iOvergaard perhaps include this in v1.14.0 release? |
Description
Fixes #1080
It doesn't render swatches in color picker for an empty array - just like in swatches itself.
I also added a story for transparent swatches using some or the original colors here:
https://github.com/umbraco/Umbraco.UI/pull/280/files#diff-57d087d5d487ba9b3141bc5c09bd619b383d6b16b09f8e422de98852789db332R25-R42
And some point the swatches in story were limited to 3 colors, but I think it would be better to have more as originally added here https://github.com/umbraco/Umbraco.UI/pull/280/files#diff-57d087d5d487ba9b3141bc5c09bd619b383d6b16b09f8e422de98852789db332R6-R23 and in color picker. Then stories cover different states of components, so it may be easier to catch any (visual) bugs.
Types of changes
Motivation and context
How to test?
Screenshots (if appropriate)
Checklist