-
Notifications
You must be signed in to change notification settings - Fork 122
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: add 4.5 contrast for text in partition slices #608
Changes from 77 commits
02f2f53
325b5bd
6acca30
8f4ddf5
23eb310
d4b43ca
361200e
963572b
01d1e2e
c7eec32
c36c79a
fe5be74
cd31813
38e1250
69168b6
c1bc892
7c3c7b0
69a2cc0
7059318
f9a824d
9bc19ba
65d8bc8
71b512e
ba1b1cf
016cc7d
9d0d6e7
c0a56e0
878635c
94d69ec
1152d96
0688de9
e083865
7ede9a3
fc86b2a
1fa7fb8
08f1dd7
a957653
876062a
9492c23
fcc7cfe
8035d6c
5b3b11f
572cbdd
336ae5e
c09dfca
08768fa
1800133
ee33cfd
4f4801b
7412a60
0a24a5e
1c04a77
511b5b4
48ec0fa
f8619e2
b411301
88e42d3
41314f9
8a4c825
0946501
545e78e
ef78e4b
299f90e
aa48c17
69cd5d8
462debe
01b7052
9bb1e8e
36baf2e
1864be2
f14f2fa
33aeb59
979ffe0
af65d11
9012b54
20a11c0
44e5101
9c91e74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,8 @@ export interface Font { | |
fontVariant: FontVariant; | ||
fontWeight: FontWeight; | ||
fontFamily: FontFamily; | ||
textColor: string; | ||
textOpacity: number; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was already possible to color the texts, would be glad to catch up on the usefulness of putting it in this type. I'm not against it and it may be the better model, I'm just curious There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. discussed over zoom |
||
|
||
export type PartialFont = Partial<Font>; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,20 +22,19 @@ import { Font } from './types'; | |
import { config, ValueGetterName } from '../config/config'; | ||
import { ArrayNode, HierarchyOfArrays } from '../utils/group_by_rollup'; | ||
import { Color } from '../../../../utils/commons'; | ||
import { LinkLabelsViewModelSpec } from '../viewmodel/link_text_layout'; | ||
import { VerticalAlignments } from '../viewmodel/viewmodel'; | ||
|
||
/** @internal */ | ||
export type LinkLabelVM = { | ||
link: PointTuples; | ||
linkLabels: PointTuples; | ||
translate: PointTuple; | ||
textAlign: CanvasTextAlign; | ||
text: string; | ||
valueText: string; | ||
width: Distance; | ||
valueWidth: Distance; | ||
verticalOffset: Distance; | ||
labelFontSpec: Font; | ||
valueFontSpec: Font; | ||
}; | ||
|
||
/** @internal */ | ||
|
@@ -95,19 +94,33 @@ export type ShapeViewModel = { | |
config: Config; | ||
quadViewModel: QuadViewModel[]; | ||
rowSets: RowSet[]; | ||
linkLabelViewModels: LinkLabelVM[]; | ||
linkLabelViewModels: LinkLabelsViewModelSpec; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call extracting it out! |
||
outsideLinksViewModel: OutsideLinksViewModel[]; | ||
diskCenter: PointObject; | ||
pickQuads: PickFunction; | ||
outerRadius: number; | ||
}; | ||
|
||
const defaultFont: Font = { | ||
fontStyle: 'normal', | ||
fontVariant: 'normal', | ||
fontFamily: '', | ||
fontWeight: 'normal', | ||
textColor: 'black', | ||
textOpacity: 1, | ||
}; | ||
|
||
/** @internal */ | ||
export const nullShapeViewModel = (specifiedConfig?: Config, diskCenter?: PointObject): ShapeViewModel => ({ | ||
config: specifiedConfig || config, | ||
quadViewModel: [], | ||
rowSets: [], | ||
linkLabelViewModels: [], | ||
linkLabelViewModels: { | ||
linkLabels: [], | ||
labelFontSpec: defaultFont, | ||
valueFontSpec: defaultFont, | ||
strokeColor: '', | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious what the goal is behind moving font/color specs from the individual There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. discussed in zoom |
||
outsideLinksViewModel: [], | ||
diskCenter: diskCenter || { x: 0, y: 0 }, | ||
pickQuads: () => [], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. */ | ||
const module = jest.requireActual('../calcs.ts'); | ||
|
||
export const getBackgroundWithContainerColorFromUser = jest.fn(module.getBackgroundWithContainerColorFromUser); | ||
export const makeHighContrastColor = jest.fn(module.makeHighContrastColor); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. */ | ||
const module = jest.requireActual('../../viewmodel/fill_text_layout.ts'); | ||
|
||
export const getTextColorIfTextInvertible = jest.fn(module.getTextColorIfTextInvertible); |
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.
If we switch to abbreviated hex code in line 173, should it happen here too? Althogh I'm not sure if it's worth the byte savings; currently it's possible to grep for strings of length 7 starting with
#
to find hex colors :-) (strings starting with#
can also be URLs etc.)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.
ah good catch thanks!