Skip to content

Connect the <PreviewFrame /> Component #1628

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

Merged
merged 10 commits into from
Oct 14, 2020
Merged

Conversation

ZenVega
Copy link
Contributor

@ZenVega ZenVega commented Oct 4, 2020

[#1590] Turning <PreviewFrame/> into a container

I added mapStateToProps() and mapDispatchToProps() to <PreviewFrame/>.
<PreviewFrame/> does not receive any props from <IDEView /> anymore.
I added connect() to the export.

Linting and npm test don't throw errors.

@ZenVega
Copy link
Contributor Author

ZenVega commented Oct 4, 2020

Hi,
this is my first time trying to commit to an open-source project. I am quite uncertain if the whole process was correct. If I got any feedback or could even contribute to p5 it would be a blast!

const mapStateToProps = state => ({
files: state.files,
htmlFile: getHTMLFile(state.files),
...state.ide,
Copy link
Member

Choose a reason for hiding this comment

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

I know that the other components that have recently been updated to use react-redux aren't like this, but I think it would be better to pull out the specific parts of the redux state that this component depends on. This is because the component will re-render if any of these change, versus the specific parts of the redux state that the component actually depends on.

The other components that are like this should be fixed too, but that's a matter for another PR 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I see, that seemed odd anyway. Ok, ill get on it.

@catarak
Copy link
Member

catarak commented Oct 6, 2020

@ZenVeg thanks for submitting this PR! Glad to have you as a contributor. It's totally okay to not do the whole code contribution process perfectly! If there's something to fix, I (or other maintainers) will tell you what to do.

@ZenVega
Copy link
Contributor Author

ZenVega commented Oct 6, 2020

Sweet, thanks. Happy to be able to give sth back to p5. Nevertheless, I can't resist asking. Would it be too much to ask to put a hacktoberfest label on that Issue?

@@ -350,4 +363,30 @@ PreviewFrame.defaultProps = {
cmController: {}
};

export default PreviewFrame;
const mapStateToProps = state => ({
files: state.files,
Copy link
Member

Choose a reason for hiding this comment

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

You're missing a bunch of props here. The ones that were passed via IDEView are:

                    htmlFile={this.props.htmlFile}	
                    files={this.props.files}	
                    content={this.props.selectedFile.content}	
                    isPlaying={this.props.ide.isPlaying}	
                    isAccessibleOutputPlaying={	
                      this.props.ide.isAccessibleOutputPlaying	
                    }	
                    textOutput={this.props.preferences.textOutput}	
                    gridOutput={this.props.preferences.gridOutput}	
                    soundOutput={this.props.preferences.soundOutput}	
                    setTextOutput={this.props.setTextOutput}	
                    setGridOutput={this.props.setGridOutput}	
                    setSoundOutput={this.props.setSoundOutput}	
                    dispatchConsoleEvent={this.props.dispatchConsoleEvent}	
                    autorefresh={this.props.preferences.autorefresh}	
                    previewIsRefreshing={this.props.ide.previewIsRefreshing}	
                    endSketchRefresh={this.props.endSketchRefresh}	
                    stopSketch={this.props.stopSketch}	
                    setBlobUrl={this.props.setBlobUrl}	
                    expandConsole={this.props.expandConsole}	
                    clearConsole={this.props.clearConsole}	
                    cmController={this.cmController}	
                    language={this.props.preferences.language}

@catarak
Copy link
Member

catarak commented Oct 7, 2020

Would it be too much to ask to put a hacktoberfest label on that Issue?

I... don't know what hacktoberfest is.

@ZenVega
Copy link
Contributor Author

ZenVega commented Oct 8, 2020

I'm not insecure about the content prop. is that correct? and are those all? do the rest come from the dispatches?

hacktoberfest is one of these events supposed to motivate people to contribute to open source and giving guidelines to achieve this. if u give this PR the label "hacktoberfest" it gets accepted and I can win a shirt or get a tree planted. that's all. that label shall prevent nonsense PRs:
https://hacktoberfest.digitalocean.com/hacktoberfest-update

@catarak
Copy link
Member

catarak commented Oct 8, 2020

I'm not insecure about the content prop. is that correct? and are those all? do the rest come from the dispatches?

If the prop was passed in IDEView, then it needs to be there for the component to work properly! If it's unclear which props are passed via mapDispatchToProps, I would recommend just importing the individual actions so it's clear what's missing, i.e.

import { stopSketch } from '../actions/ide';

// ...

const mapDispatchToProps = dispatch => bindActionCreators(
  {
    stopSketch,
    // the other actions...
  },
  dispatch
);

@ZenVega
Copy link
Contributor Author

ZenVega commented Oct 9, 2020

Ok, I imported all dispatches individually so the only prop i didn't find was cmController which I again passed via IDEView. that right?



const mapDispatchToProps = dispatch => bindActionCreators(
Object.assign(
Copy link
Member

Choose a reason for hiding this comment

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

This isn't right, as these functions aren't getting passed to the PreviewFrame component and the app crashes when you click play. You shouldn't need to use Object.assign here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I transformed mapDispatchToProps to an object. the tests pass and the editor seems to run now

Copy link
Member

Choose a reason for hiding this comment

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

working for me now! thank you ✨

@catarak catarak merged commit f0bd9ef into processing:develop Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants