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

Unexpected behavior note #450

Closed
wants to merge 13 commits into from
Closed

Unexpected behavior note #450

wants to merge 13 commits into from

Conversation

alflennik
Copy link
Contributor

@alflennik alflennik commented Jul 26, 2022

Makes the unexpected behavior note available for all unexpected behaviors instead of just "Other."

Goes along with a PR in the ARIA-AT repo which is here: w3c/aria-at#802 The two PRs can be merged in any order and at different times if needed. But following review changes to one might require changes to the other.

@alflennik alflennik changed the title WIP: Unexpected behavior note Unexpected behavior note Aug 9, 2022
@alflennik alflennik requested review from evmiguel and howard-e August 9, 2022 00:40
unexpectedBehavior,
unexpectedBehaviorNote
) => {
if (!unexpectedBehaviorNote && unexpectedBehavior.id === 'OTHER') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking: Are we still expecting a behavior note to be filled when OTHER? I thought the note would be decoupled from all undesirable behaviors.

...UNEXPECTED_BEHAVIORS.map(description => ({description, requireExplanation: false})),
{description: "Other", requireExplanation: true},
...UNEXPECTED_BEHAVIORS.map(description => ({ description, requireExplanation: false })),
{ description: 'Other', requireExplanation: true },
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here. Is Other supposed to be coupled to the explanation?

)
),
fragment(
// TODO: Figure out why this isn't appearing
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is appearing!

Copy link
Contributor

@evmiguel evmiguel left a comment

Choose a reason for hiding this comment

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

Looking good! There's a bug in the form validation.

Screen.Recording.2022-08-11.at.4.25.10.PM.mov

UPDATE: Actually, the bug persists when switching between No and Yes.

resultUnexpectedBehavior.behaviors.some(
({ checked, requireExplanation }) => requireExplanation && checked
),
description: resultUnexpectedBehavior.behaviors.some(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing this. How do I get to this state?

@alflennik
Copy link
Contributor Author

Superseded by #596

@alflennik alflennik closed this Mar 12, 2024
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.

2 participants