Add color picker to bits #782
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
.../src/components/demo/color-picker/color-picker-basic/color-picker-basic.example.component.ts
Show resolved
Hide resolved
.../components/demo/color-picker/color-picker-palette/color-picker-palette.example.component.ts
Show resolved
Hide resolved
...rc/components/demo/color-picker/color-picker-select/color-picker-select.example.component.ts
Show resolved
Hide resolved
| import trimStart from "lodash/trimStart"; | ||
|
|
||
| export function getColorValueByName(colorVariable: string): string { | ||
| if (!startsWith(colorVariable, "var(")) { |
There was a problem hiding this comment.
why not colorVariable.startsWith?
| if (!startsWith(colorVariable, "var(")) { | ||
| return colorVariable; | ||
| } | ||
| const colorName = trim(trimEnd(trimStart(colorVariable, "var("), ")")); |
There was a problem hiding this comment.
Maybe we shouldn't rely on lodash so much. useing regex might be better here, maybe something like /var\((.*)\)/
| class="color-picker-select-label" | ||
| [displayValueTemplate]="colorPickerTemplate" | ||
| > | ||
| <div *ngFor="let item of palette"> |
There was a problem hiding this comment.
let's not mix old and new control flow syntax
| templateUrl: "./color-picker.component.html", | ||
| styleUrls: ["./color-picker.component.less"], | ||
| changeDetection: ChangeDetectionStrategy.OnPush, | ||
| standalone: false, |
There was a problem hiding this comment.
Not a big issue but I'm for new components being standalone. What do you think @pavlo-poimanov
| /** | ||
| * Colors to be displayed as array of strings | ||
| */ | ||
| @Input() colors: string[]; |
There was a problem hiding this comment.
maybe this could be a required input.
(We could also start using signals, but I'd say that you can leave it like this for now)
| public registerOnTouched(fn: () => void): void { | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-empty-object-type | ||
| public registerOnTouched(fn: () => {}): void { |
There was a problem hiding this comment.
why did you change void to {}? If you wanted "object" you can use object type instead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Frontend Pull Request Description
NUI-6260
Move color picker from dashboard to bits and add new variant of color picker
Checklist
Screenshots (if applicable)
Additional Context (if necessary)