-
Notifications
You must be signed in to change notification settings - Fork 1
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
Replay button should be present even after audio prompt stops playing #192
Conversation
@@ -2,15 +2,17 @@ import { PageAudioHandler } from "./audioHandler"; | |||
import { PageStateHandler } from "./PageStateHandler"; | |||
|
|||
|
|||
export async function setupReplayAudio(pageStateHandler: PageStateHandler) { | |||
export async function setupReplayAudio(pageStateHandler: PageStateHandler, noAudio: boolean = false) { |
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.
You have the right approach. I am thinking whether we move this behavior to the PageStateHandler
itself. Because the PageStateHandler
is responsible for pagestate and setupReplayAudio
function just listens to pagestate and decides the audio. Potentially we can do something like this:
In PageStateHandler.tsx:
constructor(audioFile: string, playStimulusOnLoad: boolean) {
this.audioFile = audioFile;
this.audioUri = mediaAssets.audio[camelize(this.audioFile)] ||
mediaAssets.audio.nullAudio;
this.getbuffer();
this.replayBtn = document.getElementById('replay-btn-revisited') as HTMLButtonElement;
this.playStimulusOnLoad = playStimulusOnLoad !== undefined ? playStimulusOnLoad : true;
}
Initialize the PageStateHandler with the noAudio
value like you did for setupReplayAudio
and then in this function:
if (pageStateHandler.replayBtn) {
if (pageStateHandler.playStimulusOnLoad){
... rest of the code
}
Let me know what you think.
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.
@zwatson2001 does Aritra's comment make sense? I'd like this one merged soon -- thanks!
@kachergis will we ever have a trial where we do not show the replay button at all (like an instruction trial or otherwise)? |
@asengupta3 there are no current tasks / instructions where we don't want the replay button, although I could imagine a language task where we don't want to allow participants to repeat the stimulus endlessly. (e.g., I don't think ROAR's Phoneme Awareness task allows repetition). Certainly default is to have the replay button. |
Okay then maybe we can rename our |
@asengupta3 the playAudioOnLoad suggestion makes sense to me - @kachergis should I maybe wait to add that as a second PR so that this one can be merged quickly? |
@@ -323,7 +323,8 @@ function doOnLoad(layoutConfigMap: Record<string, LayoutConfigType>) { | |||
|
|||
const stim = taskStore().nextStimulus; | |||
const itemLayoutConfig = layoutConfigMap?.[stim.itemId]; | |||
const pageStateHandler = new PageStateHandler(stim.audioFile); | |||
const noAudio = layoutConfigMap?.[stim.itemId].noAudio; |
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.
You can just do itemLayoutConfig?.noAudio
here.
</button> | ||
`; | ||
} | ||
|
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 think this function no longer uses the noAudio
so that can be removed.
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 forgot about that, thanks!
I think that is fine. The rename can wait till next PR. |
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.
LGTM!
This PR keeps the replay button on the screen even for trials where the audio prompt is silenced. It also prevents it from being disabled at first using the noAudio parameter stored in layoutConfigMap.