Skip to content
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

Error should be raised when we try to set slider of output variable [#19] #35

Closed
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
117 changes: 68 additions & 49 deletions package-lock.json

Large diffs are not rendered by default.

22 changes: 20 additions & 2 deletions src/actions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,17 @@
*/

import * as types from '../constants/action-types';
import { setSliderByIndex, getImageData } from '../lib/calc-helpers';
import {
setSliderByIndex,
getImageData,
getSliderExpressions
} from '../lib/calc-helpers';
import { startTimer, clearTimer } from '../lib/timer';
import {
gifCreationProblem,
badBurstInput,
badSettingsInput
badSettingsInput,
noSlidersFound
} from '../lib/error-messages';
import { getBurstErrors, getSettingsErrors } from '../lib/input-helpers';

Expand Down Expand Up @@ -217,3 +222,16 @@ export const generateGIF = (images, opts) => (dispatch, getState) => {
}
});
};

export const getBurstSliders = () => dispatch => {
const sliders = getSliderExpressions();
if (!sliders.length) {
dispatch(flashError(noSlidersFound()));
}
dispatch(updateBurstSliders(sliders));
};

const updateBurstSliders = sliders => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

In the rest of this file, the actions appear to be at the top. Is there a particular reason you've placed this one apart from the rest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! moved from thunk section to action section

type: types.UPDATE_BURST_SLIDERS,
payload: sliders
});
10 changes: 10 additions & 0 deletions src/components/Burst.css
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,16 @@
text-align: center;
}

.Burst-dropdown {
height: 33px;
width: 114px;
margin-bottom: 15px;
margin-top: 5px;
border: 1px solid #000;
font-size: 1em;
outline: none !important;
Copy link
Contributor

@misscoded misscoded Jun 21, 2019

Choose a reason for hiding this comment

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

What was the reason behind this (outline)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should eliminate the blue outline that appears on focus to follow the same styling as other gifsmos inputs, e.g. .Burst-input and .Burst-button screenshots related:
Screen Shot 2019-06-21 at 11 42 43 AM
Screen Shot 2019-06-21 at 11 42 58 AM

}

.Burst-input-error {
border-color: #cc0000;
}
Expand Down
31 changes: 19 additions & 12 deletions src/components/Burst.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@ import './Burst.css';
class Burst extends Component {
constructor(props) {
super(props);

this.state = {
idx: 1,
idx: null,
min: -10,
max: 10,
step: 1,
Expand Down Expand Up @@ -38,24 +37,32 @@ class Burst extends Component {
}

render() {
const { idx, min, max, step, errors } = this.state;
const { expanded } = this.props;
const { min, max, step, errors } = this.state;
const { expanded, burstSliders } = this.props;

if (!expanded) return <div className="Burst" />;

return (
<div className={classNames('Burst', { 'Burst-expanded': expanded })}>
<div>Slider Index</div>
<input
className={classNames('Burst-input', {
'Burst-input-error': !!errors.idx
})}
type="number"
<div>Slider</div>
<select
className="Burst-dropdown"
name="idx"
aria-label="slider index"
value={isNaN(idx) ? '' : idx}
onChange={this.handleInputUpdate}
/>
>
<option value={null} defaultValue>
{burstSliders.length ? 'Choose Slider' : 'No Sliders'}
Copy link
Contributor

Choose a reason for hiding this comment

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

can you change the 'Choose Slider' text to something shorter? the text is getting cut off:

Screen Shot 2019-06-25 at 11 38 27 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to Pick Slider, now it fits

</option>
{burstSliders.map(exp => {
return (
<option key={`slider-${exp.id}`} value={exp.id}>
{' '}
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

{exp.latex.split('=').join(' = ')}
</option>
);
})}
</select>
<div>Slider Min</div>
<input
className={classNames('Burst-input', {
Expand Down
3 changes: 2 additions & 1 deletion src/components/Sidebar.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@ class Sidebar extends Component {
}

handleToggleBurst() {
const { togglePane } = this.props;
const { togglePane, getBurstSliders, expandedPane } = this.props;
togglePane(panes.BURST);
if (expandedPane !== 'BURST') getBurstSliders();
}

handleToggleSettings() {
Expand Down
1 change: 1 addition & 0 deletions src/constants/action-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export const PLAY_PREVIEW = 'PLAY_PREVIEW';
export const PAUSE_PREVIEW = 'PAUSE_PREVIEW';
export const SET_ERROR = 'SET_ERROR';
export const CLEAR_ERROR = 'CLEAR_ERROR';
export const UPDATE_BURST_SLIDERS = 'UPDATE_BURST_SLIDERS';

// Settings
export const UPDATE_IMAGE_SETTING = 'UPDATE_IMAGE_SETTING';
Expand Down
3 changes: 2 additions & 1 deletion src/containers/BurstContainer.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ const mapStateToProps = (state, ownProps) => {
expanded: ui.expandedPane === panes.BURST,
width,
height,
oversample
oversample,
burstSliders: ui.burstSliders
};
};

Expand Down
5 changes: 3 additions & 2 deletions src/containers/SidebarContainer.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { connect } from 'react-redux';
import Sidebar from '../components/Sidebar';
import { requestFrame, togglePane, reset } from '../actions';
import { requestFrame, togglePane, reset, getBurstSliders } from '../actions';

const mapStateToProps = (state, ownProps) => {
const { images, settings, ui } = state;
Expand All @@ -23,7 +23,8 @@ const SidebarContainer = connect(
{
requestFrame,
togglePane,
reset
reset,
getBurstSliders
}
)(Sidebar);

Expand Down
9 changes: 9 additions & 0 deletions src/lib/calc-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,12 @@ export const setSliderByIndex = (idx, val) => {
const identifier = match[1];
calculator.setExpression({ id, latex: `${identifier}=${val}` });
};

export const getSliderExpressions = () => {
return calculator
.getExpressions()
.filter(
exp =>
exp.latex && exp.latex !== '' && exp.latex.match(/[a-z]/gi).length === 1
);
};
4 changes: 3 additions & 1 deletion src/lib/error-messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ export const noSuchExpression = idx =>
export const notASlider = idx =>
`Looks like expression ${idx} doesn't define a slider.`;

export const noSlidersFound = () => `Looks like the graph has no sliders.`;

export const gifCreationProblem = () =>
'There was a problem creating your GIF. :(';

Expand Down Expand Up @@ -40,7 +42,7 @@ export const badBurstInput = errors => {
}

if (propText === propMap.idx)
return `Your ${propText} must be a positive integer.`;
return `Please choose a slider or define an expression`;

return `Your ${propText} isn't quite right.`;
};
Expand Down
2 changes: 1 addition & 1 deletion src/lib/input-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export const getBurstErrors = inputs => {
if (isNaN(inputs[prop])) errors[prop] = true;
}

if (!isPositiveInteger(idx)) errors.idx = true;
if (!idx) errors.idx = true;
if (min >= max) {
errors.min = true;
errors.max = true;
Expand Down
14 changes: 12 additions & 2 deletions src/reducers/ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* previewIdx: the index of the frame being previewed in the preview pane
* playing: whether the preview is animating
* error: currently displayed error message
* burstSliders: expressions from the calculator that are valid sliders
*/

import {
Expand All @@ -15,15 +16,17 @@ import {
PAUSE_PREVIEW,
SET_ERROR,
CLEAR_ERROR,
RESET
RESET,
UPDATE_BURST_SLIDERS
} from '../constants/action-types';
import panes from '../constants/pane-types';

const initialState = {
expandedPane: panes.NONE,
previewIdx: 0,
playing: false,
error: ''
error: '',
burstSliders: []
};

const ui = (state = initialState, { type, payload }) => {
Expand Down Expand Up @@ -88,6 +91,13 @@ const ui = (state = initialState, { type, payload }) => {
}
};

case UPDATE_BURST_SLIDERS: {
return {
...state,
burstSliders: payload
};
}

default:
return state;
}
Expand Down