Skip to content

release/react #18

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.

Sign up for GitHub

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

Open
TonyBrobston wants to merge 22 commits into
base: release/react
Choose a base branch
from
Open

release/react #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
bb481ae
Switching to an arrow function
TonyBrobston Sep 1, 2018
51d711c
Switching how we set selectedTracks
TonyBrobston Sep 1, 2018
598b10a
Return empty array if tracks is null, might have the syntax wrong
TonyBrobston Sep 1, 2018
3204fbe
Return empty array if tracks is null
TonyBrobston Sep 1, 2018
bfb231f
Only display disc info if there are tracks
TonyBrobston Sep 1, 2018
5ac0c48
Default tracks to empty
TonyBrobston Sep 1, 2018
c6d79d4
Check that tracks exist first
TonyBrobston Sep 1, 2018
f59c033
Name buttons and adjust how we select tracks
TonyBrobston Sep 1, 2018
9ee719f
Name buttons and adjust how we select tracks
TonyBrobston Sep 1, 2018
564bbbe
Trying to figure out what is on event
TonyBrobston Sep 1, 2018
e4da1d2
Trying to figure out what is on event
TonyBrobston Sep 1, 2018
3c624b5
Trying to determine which checkbox is checked
TonyBrobston Sep 1, 2018
8b9887a
Switching id for name
TonyBrobston Sep 1, 2018
41d49d3
Trying to populate selected track ids
TonyBrobston Sep 1, 2018
6faef78
Defaulting isSelected to true won't work
TonyBrobston Sep 1, 2018
869ff2f
Change how we determine if it is checked
TonyBrobston Sep 1, 2018
0831d64
Change how we determine whether we prefill the checkbox or not
TonyBrobston Sep 1, 2018
b0217a7
Switch foreach for map
TonyBrobston Sep 1, 2018
0423b38
Map to foreach, not sure if I can use a reduce here? or maybe a filter?
TonyBrobston Sep 2, 2018
6fe4914
Need to pass driveId down
TonyBrobston Sep 2, 2018
5bd511a
Passing down discName
TonyBrobston Sep 2, 2018
e46b07c
Fixing an incorrect method name and fixing how we check if a set has …
TonyBrobston Sep 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 34 additions & 53 deletions client/src/DiscInfo/DiscInfo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,71 +1,50 @@
import React, {Component} from 'react';
import PropTypes from 'prop-types';
import $ from 'jquery';

import {
Button,
Form,
FormGroup,
Input,
Label,
Table,
} from 'reactstrap';

import {
actionRipTracks,
} from '../api.js'
import {Button, Form, FormGroup, Input, Label, Table,} from 'reactstrap';

import {actionRipTracks,} from '../api.js'

class DiscInfo extends Component {

constructor(props) {
super(props);
let selectedTracks = {};
this.props.tracks && this.props.tracks.map((trackInfo, trackId) => {
selectedTracks[trackId] = trackInfo.isAutoSelected;
});
this.state = {
checkAll: false,
discName: false,
selectedTracks: selectedTracks,
selectedTracks: this.props.tracks.map((trackInfo) => {
return trackInfo;
})
};
}

getTrackCheckboxes($formElement) {
return $formElement
.closest('fieldset')
.find('input[name=selectTrack]');
}

// Toggle the checkbox on all tracks.
toggleAllTracks(event) {
let $target = $(event.target);
this.setState({
checkAll: $target.prop('checked'),
});
this.getTrackCheckboxes(event.target)
.prop('checked', this.state.checkAll);
alert('This functionality is currently disabled.');
Copy link
Author

@TonyBrobston TonyBrobston Sep 2, 2018

Choose a reason for hiding this comment

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

I'll add this back once I get ripping working.

// let $target = $(event.target);
// this.setState({
// checkAll: $target.prop('checked'),
// });
// this.getTrackCheckboxes(event.target)
// .prop('checked', this.state.checkAll);
}

toggleTrack(trackId) {
let changeObj = {};
changeObj[trackId] = !this.selectedTracks[trackId];
this.setState({
selectedTracks: Object.assign(
this.selectedTracks, changeObj
)
})
toggleTrack(event) {
let trackId = event.target.name;
this.state.selectedTracks[trackId].isSelected = event.target.checked;
}

// Command the server to rip certain tracks for this disc.
ripTracks(event) {
let ripTrackIds = this.getTrackCheckboxes(event.target)
.find(':checked')
.data('track-id');
ripTracks() {
let trackIds = [];
this.state.selectedTracks.forEach((selectedTrack, trackId) => {
if (selectedTrack.isSelected) {
trackIds.push(trackId);
}
});
Copy link
Author

@TonyBrobston TonyBrobston Sep 2, 2018

Choose a reason for hiding this comment

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

I was hoping to use a .map( function here, however for every track that I didn't return it would default to undefined. The result I wanted was something like: ['1', '5'], however map would produce [undefined, '1', undefined, undefined, undefined, '5']. I'm wondering if a filter or reduce method would be a better option.

Copy link
Owner

@lasley lasley Oct 13, 2018

Choose a reason for hiding this comment

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

Yep you want reduce

actionRipTracks(
this.state.discName,
this.props.discName,
this.props.driveId,
ripTrackIds
trackIds
);
}

Expand Down Expand Up @@ -109,14 +88,14 @@ class DiscInfo extends Component {
</tr>
</thead>
<tbody>
{this.props.tracks && this.props.tracks.map(function(trackInfo, trackId) {
return <tr onClick={ (e) => this.toggleTrack(trackId) }>
{this.props.tracks && this.props.tracks.map((trackInfo, trackId) => {
return <tr>
<td>
<Input type="checkbox"
name="selectTrack"
checked={ this.state.selectedTracks[trackId] }
onChange={ (e) => this.toggleTrack(trackId) }
/>s
name={trackId}
checked={this.state.selectedTracks[trackId].isSelected}
onChange={(event) => this.toggleTrack(event)}
Copy link
Author

@TonyBrobston TonyBrobston Sep 2, 2018

Choose a reason for hiding this comment

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

If I set the name to the trackId and used the event, I was able to determine if a track was selected without using jQuery, which I thought was nice; we could likely eliminate jQuery completely.

Copy link
Owner

@lasley lasley Oct 13, 2018

Choose a reason for hiding this comment

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

Yeah jQuery is superfluous once React comes into play. I just didn't quite realize it back then ;)

/>
</td>
<td>{ trackInfo.orderWeight }</td>
<td>{ trackInfo.name }</td>
Expand All @@ -130,7 +109,9 @@ class DiscInfo extends Component {
</Table>
</FormGroup>
<FormGroup>
<Button onClick={ (e) => this.ripTracks(e) } />
<Button onClick={() => this.ripTracks()}>
Rip Tracks
</Button>
</FormGroup>
</fieldset>
</Form>
Expand All @@ -152,7 +133,7 @@ DiscInfo.propTypes = {
volumeName: PropTypes.string.isRequired,
tracks: PropTypes.arrayOf(PropTypes.shape({
id: PropTypes.number.isRequired,
isAutoSelected: PropTypes.bool,
isSelected: PropTypes.bool,
ripStatus: PropTypes.oneOf(['none', 'busy', 'fail', 'success']),
chapterCount: PropTypes.number.isRequired,
diskSize: PropTypes.string.isRequired,
Expand Down
23 changes: 15 additions & 8 deletions client/src/DiscPanel/DiscPanel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class DiscPanel extends Component {
constructor(props) {
super(props);
this.state = {
discInfo: {},
discInfo: {}
};
subscribeToDiscInfo(this.handleDiscInfo, this, this.props.driveId);
}
Expand All @@ -39,27 +39,34 @@ class DiscPanel extends Component {
}

render(){
let discInfo = '';
if (this.state.discInfo.tracks && this.state.discInfo.tracks.length > 0) {
Copy link
Author

@TonyBrobston TonyBrobston Sep 2, 2018

Choose a reason for hiding this comment

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

I needed to conditionally display <DiscInfo />. If DiscInfo's constructor fired without tracks available, I got some wonky behavior.

Copy link
Owner

@lasley lasley Oct 13, 2018

Choose a reason for hiding this comment

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

Better way to do this would be something like:

discInfo = () => {
  if(...) {
    return null
  }
  return <DiscInfo ... />
}

Then just discInfo() when using

discInfo = <DiscInfo
driveState={this.props.driveState}
driveId={this.props.driveId}
discName={this.props.discName}
{...this.state.discInfo}
/>;
}
return(
<div className="DiscPanel">
<Card>
<CardBody>
<CardTitle>
<span>
{ this.props.driveId }
{this.props.driveId}
</span>
&nbsp;-&nbsp;
<span>
{ this.props.discName || 'No Disc' }
{this.props.discName || 'No Disc'}
</span>
</CardTitle>
<Button onClick={ () => this.refreshDiscInfo() }>
<span className="glyphicon glyphicon-refresh" />
<Button onClick={() => this.refreshDiscInfo()}>
Refresh Disc
</Button>
</CardBody>
<CardBody>
<DiscInfo driveState={ this.props.driveState }
{ ...this.state.discInfo }
/>
{discInfo}
</CardBody>
</Card>
</div>
Expand Down
4 changes: 2 additions & 2 deletions makemkv.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ class MakeMkv {
trackIds.map((trackId) => this.ripQueue.driveId.add(trackId));
}

if (!this.ripQueue.driveId.length) {
if (!this.ripQueue.driveId.size) {
Copy link
Author

@TonyBrobston TonyBrobston Sep 5, 2018

Choose a reason for hiding this comment

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

Kind of confused. driveId is a Set, but you're interacting with it like it's an array. Not sure which you meant for it to be, so I stuck with a set.

Copy link
Owner

@lasley lasley Oct 13, 2018

Choose a reason for hiding this comment

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

Yep this was wrong. Looks like the conditional doesn't do anything, so not really sure what I was going for honestly

// @TODO: What to do here?
return;
}
Expand All @@ -224,7 +224,7 @@ class MakeMkv {
};

this.ripTrack(
saveDirectory, driveId, this.ripQueue.driveId.pop(), newCallback
saveDirectory, driveId, Array.from(this.ripQueue.driveId).pop(), newCallback
Copy link
Author

@TonyBrobston TonyBrobston Sep 5, 2018

Choose a reason for hiding this comment

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

Hoping this actually removes the last element from the set and returns it; rather than just returning it. Hoping to test drive some of this stuff later on, but just trying to get things working quick-and-dirty for now with minimal change.

Copy link
Owner

@lasley lasley Oct 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't, and you can't access sets by index either. Better would probably be to just switch the set back to an array - it was just to clear up an edge case when someone submits the same track twice. It would only be possible if there are two UIs open talking to the server at the same time, and both submit the same track for ripping (within milliseconds of each other). Totally not going to happen outside of a lab

Copy link
Owner

@lasley lasley Oct 13, 2018

Choose a reason for hiding this comment

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

But if you did want to handle this:

const ripTracks = [...this.ripQueue.driveId];
const ripTrack = ripTracks.pop();
this.ripQueue.driveId.delete(ripTrack);

Seems like a lot of work though, when we could accomplish the same functionality by just guarding the array push for the track

);

}
Expand Down
4 changes: 2 additions & 2 deletions server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ class MakeMkvServer {

// Actions
client.on('doDiscInfo', (data) => { this.doDiscInfo(data) });
client.on('doRipTrack', (data) => { this.doRipTrack(data) });
client.on('doRipTracks', (data) => { this.doRipTracks(data) });

}

Expand All @@ -225,7 +225,7 @@ class MakeMkvServer {
this.makeMkv.getDiscInfo(driveId, callback);
}

doRipTrack(data) {
doRipTracks(data) {

let driveId = data.driveId,
discName = data.discName,
Expand Down