-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat(chrome-ext): palette generator #1663
Conversation
8171117
to
9b69b56
Compare
f8b0a16
to
28796ea
Compare
apps/chrome-devtools/src/app-devtools/theming-panel/theming-panel-pres.component.ts
Outdated
Show resolved
Hide resolved
28796ea
to
a07e1c4
Compare
fbefd1a
to
baf84c7
Compare
</div> | ||
<label class="form-label text-nowrap fw-semibold my-0 ms-4" [for]="group.name">{{ group.name }}</label> | ||
@if (form.value.variables?.[group.defaultVariable.name] !== group.defaultVariable.defaultValue) { | ||
<i class="form-text text-warning icon-undo position-absolute end-0 me-10" ngbTooltip="Reset all colors of {{group.name}}" (click)="onPaletteReset(group, $event)"></i> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good practice in terms of accessibility to have a clickable element inside the header of the accordion which is already clickable
apps/chrome-devtools/src/app-devtools/theming-panel/color.pipe.ts
Outdated
Show resolved
Hide resolved
<span class="my-auto">{{ resolvedColor }}</span> | ||
<span class="ms-auto my-auto"> | ||
<span ngbTooltip="Accessibility score for small text" [style.fontSize]="'12px'" class="me-2">{{ resolvedColor | accessibilityConstrastScore : (resolvedColor | contrast) : 'small' }}</span> | ||
<span ngbTooltip="Accessibility score for large text" [style.fontSize]="'24px'">{{ resolvedColor | accessibilityConstrastScore : (resolvedColor | contrast) : 'large' }}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use style.fontSize and use a hard coded value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hardcoded value because for accessibility large text start at 24px and for small text I picked 12px to let the user see a big difference between large and small without having it too small to be read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for [style.fontSize]="'24px'"
I found it clearer than style="{font-size: 24px'}
and Angular provide autocomplete for [style.<...>]
baf84c7
to
4308fa3
Compare
68f590f
to
e38991b
Compare
e38991b
to
ddd43f4
Compare
@@ -94,6 +95,7 @@ | |||
"@o3r/components": "workspace:^", | |||
"@o3r/configuration": "workspace:^", | |||
"@o3r/core": "workspace:^", | |||
"@o3r/design": "workspace:^", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True I will fix it in another PR as this one is merged :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
standalone: true | ||
}) | ||
export class ColorPipe implements PipeTransform { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an internal pipe expose only in this folder
Proposed change
Related issues