-
Notifications
You must be signed in to change notification settings - Fork 28
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
Support multiple settings being defined in *-commands.csv
#1125
base: master
Are you sure you want to change the base?
Conversation
cc @mcking65 |
) { | ||
const commandInfo = { | ||
testId: command.testId, | ||
command: command.command, | ||
settings: command.settings || 'defaultMode', | ||
settings: primarySetting, |
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.
This definition of settings
isn't really appropriate in the presence of additionalSettings
. Will this change also be reflected in the test format definition? What would be the ramifications of renaming this property to primarySetting
?
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.
This definition of
settings
isn't really appropriate in the presence ofadditionalSettings
... renaming this property toprimarySetting
?
I agree! I figured as much when sharing the initial gist on what's different here. I proposed mainSetting
then and have since used primarySetting
here as well which feels more appropriate. Otherwise, it certainly presents a bit of confusion without this context.
Will this change also be reflected in the test format definition?
I don't think it necessary. Seems to be something to consider as an implementation detail.
What would be the ramifications ...
A widespread refactor across the references in this repository, aria-at-app (as well as some migration or backward compatibility work) and also in aria-at-automation-harness and related resources I assume. That will certainly need a bit of coordination that I'm uneasy trying to get that done now. Do you feel strongly otherwise?
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 agree that it's an implementation detail for the Community Group at large. That said, the amount of inter-project coordination needed to support change makes it feel like a public-facing design.
This change creates a gap between what the CG calls "settings" and what code maintainers call "settings", and to me, that's a problem which is distinct from the internally inconsistent names.
I dropped in from w3c/aria-at-automation-harness#73 to verify that my understanding of the situation was accurate. I trust your judgement when it comes to coordinating changes; I'd only advocate for prioritizing that refactoring sooner rather than later!
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.
Looks great! I reviewed locally and tested making changes. The additional settings render as expected. Left a few comments inline but nothing blocking. Thanks Howard!
// TODO: Account for a list of settings as described by https://github.com/w3c/aria-at/wiki/Test-Format-Definition-V2#settings | ||
const settingsValue = command.settings || 'defaultMode'; | ||
const [primarySetting, ...additionalSettings] = command.settings | ||
? command.settings.split(' ') |
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.
This structure with the primary setting as the first item in a space-separated collection feels a bit fragile. I'd rather have this as a distinct column. It feels like there is significance not surfaced by a quick read of the input. I assume that this is a design decision towards a better authoring experience so I support leaving as-is for now
c.settings === assertionForCommand.settings | ||
c.settings === assertionForCommand.settings && | ||
c.presentationNumber === assertionForCommand.presentationNumber && | ||
c.assertionExceptions.includes(assertion.assertionId) |
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.
Nice!
@@ -375,58 +375,58 @@ class CommandsInput { | |||
return commands; | |||
} | |||
|
|||
getCommandsV2({ task }, mode) { | |||
getCommandsV2(task, mode) { |
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.
👍
), | ||
}; | ||
additionalSettingsExpanded.push(expandedSettings); | ||
} |
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.
Wonder if this can be cleanly combined with the similar code above?
} = commandSettings[index]; | ||
|
||
const hasScreenText = screenText && settings !== 'defaultMode'; | ||
const hasadditionalSettings = hasScreenText && additionalSettingsExpanded.length > 0; |
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 go back and forth on this. I get the logic of keeping the camel case from the additionalSettings
but having a variable without normal capitalization does feel odd
screenTextRender = `${screenTextRender} and ${settingsText}`; | ||
}); | ||
} | ||
screenTextRender = `${screenTextRender})`; |
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.
Could also possibly be combined with the similar code above?
Preview Tests
Address #1002.
Note: Created #1126 to test the changes on
tests/alert/data/jaws-commands.csv
(these are not meant to replicate real world defined settings or scenarios):speechRateIncrease
andcommentAnnouncementOn
setting. Also added a newEnter + virtualCursor + commentAnnouncementOn
commandNote2: This work should NOT be merged until a supporting PR is included on w3c/aria-at-app as the resulting test format output is being changed to have an
additionalSettings
property.