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

[contrast] Add contrast methods #194

Merged
merged 24 commits into from
Jul 31, 2022
Merged

[contrast] Add contrast methods #194

merged 24 commits into from
Jul 31, 2022

Conversation

svgeesus
Copy link
Member

@svgeesus svgeesus commented Jul 13, 2022

Done:

  • Add Weber, Michelson, Lstar and APCA contrasts in new /src/contrast folder
  • Add and rename contrast to contrastWCAG21, in src/contrast

ToDo:

  • more testing
  • write tests page to check expected values
  • generic "contrast" method that takes a "method" parameter and dispatches to one of these
  • default value for "method"
  • Contrast documentation

@svgeesus svgeesus marked this pull request as draft July 13, 2022 16:02
@svgeesus svgeesus requested a review from LeaVerou July 13, 2022 16:02
@svgeesus
Copy link
Member Author

@LeaVerou any thoughts so far?

@svgeesus
Copy link
Member Author

I added documentation. Currently using the procedural API as the functions have not been object-oriented

@svgeesus
Copy link
Member Author

Checking for Y=0 is certainly worthwhile as this is likely to be common. These changes would affect Weber and Michelson

Negative Y is unlikely but possible and, as you said, clamping to 0 seems best. That would affect anything luminance-based.

docs/contrast.md Outdated Show resolved Hide resolved
src/contrast/APCA-098G.js Outdated Show resolved Hide resolved
src/contrast/APCA-098G.js Outdated Show resolved Hide resolved
@svgeesus
Copy link
Member Author

So I have

export default function contrastAPCA(foreground, background) {

and then

export function register(Color) {
	Color.defineFunction("contrastAPCA", contrastAPCA);
}

but what I really want is to end up with

Color.contrastAPCAText(background);
Color.contrastAPCABackground(foreground);

which will produce

 contrastAPCA(this, background);
 contrastAPCA(foreground, this);

respectively.

@LeaVerou
Copy link
Member

I assume you mean color.contrastAPCAText(background), i.e. instance methods.

I don't think this is a good design. Long method names are hard to read, and if I see color1.contrastAPCAText(color2) I'm still unsure what's the foreground and what's the background.

To borrow from the proposed keywords for color-contrast(), perhaps color1.contrastOver(color2) and color1.contrastUnder(). Or even a single method, that accepts an object literal:

color1.contrast({foreground: color2}), color1.contrast({background: color2})
color1.contrast({over: color2}), color1.contrast({under: color2})

@svgeesus
Copy link
Member Author

svgeesus commented Jul 20, 2022

To borrow from the proposed keywords for color-contrast(), perhaps color1.contrastOver(color2) and color1.contrastUnder().

It seems pretty important at this stage to include at minimum APCA in the method name as well (and may be too soon to call it that, rather than APCA-0.0.98G)

@svgeesus
Copy link
Member Author

Added tests for the APCA function, based on eight known-good results from the apca-w3 test. Seven are passed (to all significant figures) one is not, and I believe his test is incorrect

@svgeesus
Copy link
Member Author

Eighth APCA test confirmed wrong, corrected

Copy link
Member

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

LGTM. We need to add a color.contrast(color, methodname) method though. Could be done in a separate PR.

@svgeesus svgeesus marked this pull request as ready for review July 31, 2022 14:08
@svgeesus svgeesus merged commit 77d31be into main Jul 31, 2022
@svgeesus svgeesus deleted the contrast branch July 31, 2022 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants