Patch/audio component #375
Conversation
|
I tested this locally. Everything works fine except for a minor visual bug when the table first loads up. That can be fixed by adjusting the window size slightly. More details in Issue #376 |
There was a problem hiding this comment.
@keving17 This looks great! One question here is how the awesome cleanup in this PR would impact other older analyses.
client/src/research/DynamicTable.js
Outdated
| return ( | ||
| <div id={this.state.audioID+"-transcript"}>Transcript: "{this.state.transcript}"</div> | ||
| // <div id={audioID+"-transcript"}>Transcript: "{results.transcript}"</div> |
client/src/research/DynamicTable.js
Outdated
| @@ -344,12 +230,105 @@ export default class DynamicHeightTableColumn extends React.PureComponent { | |||
| dataKey="responseText" | |||
| label="Response" | |||
| disableSort = {false} | |||
| cellDataGetter={({ dataKey , rowData }) => rowData.json[dataKey] || (this._getAudioUrl(rowData) && <div>{this._loadAudioPlayer(this._getAudioID(this._getAudioUrl(rowData)))} {this._loadTranscript(this._getAudioID(this._getAudioUrl(rowData)))} </div>)} | |||
| cellRenderer= {this._wrappingCellRendererAgain} | |||
| cellDataGetter={({ dataKey , rowData }) => rowData.json[dataKey] || (this._getAudioUrl(rowData) && <div><AudioPlayerComponent audioID={this._getAudioID(this._getAudioUrl(rowData))} token={this.props.token} /> <TranscriptComponent audioID={this._getAudioID(this._getAudioUrl(rowData))} token={this.props.token}/> </div>)} | |||
There was a problem hiding this comment.
This is a lot in one line :) maybe make a method that cellDataGetter can call?
| // For looking at raw events and sessions. Hover to reveal more sensitive data. | ||
| renderEventsTable(json) { | ||
| const {analysisKey} = this.props; | ||
| if (analysisKey === 'HMTCA') return this.renderHmtcaTable(json); |
There was a problem hiding this comment.
This looks like it's simplifying, but also that it's removing some code that is used to analyze other data besides the scenarios you've been focused on. Can you say more about how removing this code impacts viewing data for other sessions or scenarios?
If removing this would break those, let's leave this code and add a branching to use the new awesome view where it works and for older analyses use this old code. Or if it didn't really work before this change to look at the older analyses, then I think it's fine to remove this and just add a comment linking back to an older commit to point future folks in a helpful direction.
There was a problem hiding this comment.
So those stopped working once I stopped using the renderEventsTable() function and switched to using react virtualized. Are you saying to leave a comment in the actual code about this?
There was a problem hiding this comment.
Yep! I think it's fine to break these older scenarios to make the awesome improvements you've made. I'd suggest just adding comment that says something like "for analysis of other scenarios, look at (this URL in the git history)" as a helpful pointer for future folks.
client/src/research/DynamicTable.js
Outdated
| } | ||
|
|
||
| class AudioPlayerComponent extends React.Component { |
There was a problem hiding this comment.
This looks great. Can you move AudioPlayerComponent and TranscriptComponent into their own files? They're now nicely self-contained which simplifies the table code quite a bit but also could be re-used if other folks are making other kinds of analysis. In the same file here they're a bit hidden away.
|
So overall, this PR should not change any of the behavior since commit a3ffebd where I fixed up some problems with using react-virtualized. I only create and move the AudioPlayerComponent and TranscriptComponent outside of the constructor for the data table and clean up some leftover code from when I switched over to react-virtualized. |
|
If things look ok to you @kevinrobinson , I can merge this. |
|
@keving17 sounds great, 🚢! |
Addresses Issues #362 and #361 about moving audio players and transcripts to their own separate component rather than finding elements by ID and updating them as data was retrieved from s3 and Watson API.
This adds two new components to
DynamicTable.js:AudioPlayerComponentandTranscriptComponentwhich update the table as data is returned.