testbed file organization #429
Conversation
Coverage report
Test suite run success144 tests passing in 16 suites. Report generated by 🧪jest coverage report action from e1c5555 |
| const getContents = () => { | ||
| if (agentData.instanceId === -1) { | ||
| return <div>No agent selected</div>; | ||
| return <div className="ui-container">No agent selected</div>; |
There was a problem hiding this comment.
Existing custom components get this wrapper to show what they contain, goal is to factor all the loose UI html into components and then get it into a scheme.
| <div className="ui-container"> | ||
| <button onClick={() => setRecordingEnabled()}> | ||
| {isRecordingEnabled ? "Disable" : "Enable"} Recording | ||
| </button> |
There was a problem hiding this comment.
Moved this into the component so that recording related things are all contained inside the ui-container class.
interim17
requested review from
frasercl and
meganrm
and removed request for
a team
frasercl
left a comment
frasercl
left a comment
There was a problem hiding this comment.
Looks good, like the organization! Suggested a couple minor edits
There was a problem hiding this comment.
Optional: while you're here, this file uses a lodash map that could be replaced by a native map
There was a problem hiding this comment.
InputSwitch has one too, and CollectionInput has a map and a reduce
There was a problem hiding this comment.
Taking suggestion to remove the lodash map from ConversionForm, but leaving the calls in InputSwitch and CollectionInput for now.
The input collections are objects not arrays, so that actually is the use case for the lodash map and reduce as I understand them and so feels out of scope to change rn.
| if ( | ||
| isEqual(uiDisplayData, this.state.selectionStateInfo.appliedColors) | ||
| ) { |
There was a problem hiding this comment.
idk if anyone's especially committed to it, but hot take this repo's 80 column limit is a bit too small
There was a problem hiding this comment.
Sizzler! I like it though... @meganrm what do you think about bumping the prettier printWidth to 120? We will vibe with volume-viewer then.
AICS style guide anyone? Would actually be nice if this sort of thing was consistent across repos.
| @@ -1,13 +1,18 @@ | |||
| import { set } from "lodash"; | |||
There was a problem hiding this comment.
linter says this is unused - accidental auto-import?
| }; | ||
|
|
||
| return <div>Agent Metadata: {getContents()}</div>; | ||
| return getContents(); |
There was a problem hiding this comment.
Can delete getContents and turn its returns into this component's returns
Time Estimate or Size
medium
Problem
Starting work on #428
Solution
Componentsdirectory:RecordMoviesComponent,ColorPicker,AgentMetadata,ConversionForm