Skip to content
This repository has been archived by the owner on Apr 14, 2022. It is now read-only.

course rendering #104

Merged
merged 28 commits into from
Sep 19, 2017
Merged

course rendering #104

merged 28 commits into from
Sep 19, 2017

Conversation

inyono
Copy link
Member

@inyono inyono commented Apr 26, 2017

Work by @igorzamyslov and @cognostics 🎉 (ping #77)


// router demonstration
const reducer = combineReducers(reducers);
const store = createStore(reducer);
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should be done in the story itself, otherwise the store is not reset when one switches between stories (and then you wonder why the forward button does not work)

Copy link
Contributor

Choose a reason for hiding this comment

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

I've ended up making a completely different story for router, which is much simpler and better represents the router functionality (and doesn't use store at all)
But yes, I agree - initialisation should've been in the story itself.

app.json Outdated
@@ -1,5 +1,13 @@
{
"expo": {
"sdkVersion": "15.0.0"
"sdkVersion": "15.0.0",
"name": "Serlo-ABC",
Copy link
Member Author

Choose a reason for hiding this comment

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

If I read the commit messages correctly, then this is used for the production build? Didn't look into how to do that yet. So if possible, could you add a npm task for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Andrej said, that he sent you the instructions already for how to build a production version of the app, I will attach it here just for history purposes - basically it just uses Expo utility for that.
But if you'll decide that it's worth to put it in separate npm task - we can always do it :)

I’ve added a configuration file to our repository to build a production version of application using Expo: first one needs to install exp (either globally with -g param or locally):
npm install exp
The run some command with it in order for it to propose to create an account (If you’ve installed it globally ./node_modules/.bin part shouldn’t be there):
./node_modules/.bin/exp start
Go through the process of creating an account, and then run:
./node_modules/.bin/exp build:android (ios)
It will take about 10-15 minutes to build an .apk file, you can check the process with:
./node_modules/.bin/exp build:status -> in the end it will write url to the newly generated application file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks :)

package.json Outdated
@@ -35,6 +35,8 @@
"expo": "^15.1.3",
"ramda": "^0.23.0",
"react": "~15.4.2",
"react-native": "^0.42.0"
"react-native": "^0.42.0",
"react-native-router-flux": "^3.38.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

To get it run on OS X, I needed to downgrade react-native-router-flux, i.e. "react-native-router-flux": "3.38.0", instead of "react-native-router-flux": "^3.38.0",

}

render () {
const { course, changeExercise, nextExercise, currentExercise } = this.props;
Copy link
Member Author

Choose a reason for hiding this comment

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

changeExercise gets passed down from connect I suppose? Could we do that in this file directly so that one knows where changeExercises comes from?

Copy link
Contributor

Choose a reason for hiding this comment

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

For the ExerciseLayout component - these functions are just the props passed from parent (/containers/ExerciseApp.js), which is in fact connected to the Redux Store.

I've used this structure as it is proposed by Redux documentation (in this case ExerciseLayout is a presentational component and ExerciseApp is a container), but we can change the structure any way you want :)

<View key={`s_${sectionIndex}_c_${chapterIndex}`}>
<Text style={styles.chapterText}>Chapter {chapter.title}</Text>
{chapter.exercises.map((exercise) => {
const eIndex = exerciseIndex;
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a reason why this can't be the index from map?

Copy link
Member Author

Choose a reason for hiding this comment

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

note /self: yes, basically it generates a global id

Copy link
Contributor

@igorzamyslov igorzamyslov Apr 27, 2017

Choose a reason for hiding this comment

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

Yes, you're right - here it's like a global id across all sections/chapters for the exercise, today I will work more on the exercises queue - so I may end up changing it a bit.

};

// init current exercise from storage
AsyncStorage
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we could use redux-persist for that since this info will be part of the store, anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good Idea, will implement it instead

…ncies - changed the version to match exactly.

For router demonstration story - I’ve completely changed the story (made it easier to understand, plus it shows the functionality better)
Added some logic to one of the exercises (DifferFromSymbol) to be able to select answer and change button colour based on the selected answer and submission status (success/fail).
Added actions to exercise-reducer to: select an answer, submit an answer for current exercise.
Now instead of generating separate queue of exercises we preserve a course tree.
@igorzamyslov
Copy link
Contributor

igorzamyslov commented Apr 27, 2017

I have checked the errors from flow, they are all caused by react-native-router-flux dependencies, there are issues about both cases, but in both issues the only solution is to ignore those errors.

Ignore code with issue-references:

; Ignore react-native-experimental-navigation errors
; https://github.com/aksonov/react-native-experimental-navigation/issues/12
.*/node_modules/react-native-experimental-navigation/.*

; Ignore react-native-router-flux errors
; https://github.com/aksonov/react-native-router-flux/issues/1465
.*/node_modules/react-static-container/.*

Should I put it into .flowconfig? Or I can remove router package for now as it's not used at the time. (But most probably some router package will be needed later)

And another quick question: I've worked today in a separate branch with exercise logic - do you want me to keep all branches separate or merge everything into master in our forked repository to see the changes here?

@inyono
Copy link
Member Author

inyono commented Apr 27, 2017

Should I put it into .flowconfig? Or I can remove router package for now as it's not used at the time. (But most probably some router package will be needed later)

Feel free to put that into .flowconfig since we aren't using flow for most parts anyway

I've worked today in a separate branch with exercise logic - do you want me to keep all branches separate or merge everything into master in our forked repository to see the changes here?

What you prefer. If you have something that is mostly done and could be merged into serlo-org/master, feel free to create another PR for that. But we can also make one big PR for that since it doesn't affect the other stuff we are working on. Your call :)

Added some logic to one of the exercises (DifferFromSymbol) to be able to select answer and change button colour based on the selected answer and submission status (success/fail).
Added actions to exercise-reducer to: select an answer, submit an answer for current exercise.
Now instead of generating separate queue of exercises we preserve a course tree.
@igorzamyslov
Copy link
Contributor

All right, I have merged all changes to master, so they were visible here. (It's basically all related to redux store and general app structure)
In the latest merge next features were added:

  • Added some logic to one of the exercises (DifferFromSymbol) to be able to select answer and change button colour based on the selected answer and submission status (success/fail).
  • Added actions to exercise-reducer to: select an answer, submit an answer for current exercise.
    (Please check if the structure is fine - These actions are passed into the exercise (DifferFromSymbol only for now) with some additional info about current state of exercise, and called from there)
  • Now instead of generating separate queue of exercises we preserve a course tree.
  • After submitting the answer you can see if the answer is correct or not and after a moment redirected to next exercise
  • When exercise submission is failed, we add failed exercise to the end of the chapter to be called again

@inyono
Copy link
Member Author

inyono commented Apr 28, 2017

Hey @igorzamyslov. Thanks for the work. I will look into that this weekend.

@inyono
Copy link
Member Author

inyono commented May 5, 2017

Hey @igorzamyslov. Sorry for the late response.

QUESTION: should it be possible to resubmit the same exercise?

Depends. There are exercises that provide feedback (like your DifferFromSymbol example) after one submits a wrong answer. Those should just be queued again.
If the exercise has no feedback, though, then some general negative feedback should be given and the user should solve the exercise again.

TODO: provide correct answer in some unified form

My initial design idea was that the logic is done by a higher-order component, see #41, so that the underlying exercise can pass the correct answer to its wrapper (that also keeps track of the state instead of the Redux store). IMHO, it's fine to use the redux store, too (although it's not needed since the "exercise state" is probably only used locally. So go with what you prefer :) ). For example, you could dispatch an action on componentDidMount with the correct answer. It should be fine as long as selectAnswer can also accept arrays/objects.

Other than that, it looks pretty good and it seems like minimal change to the exercises is needed 👍

@inyono
Copy link
Member Author

inyono commented May 12, 2017

Hey @igorzamyslov. Just wanted to check if there are any updates?

* serlo_org_master:
  chore: remove `yarn flow` from README
  chore: update dependencies, standalone build, run prettier on precommit
  RoundText: transparent background for Text
  exercises: LetterRotated (serlo#99)
  exercise: VideoQuestion (serlo#105)

# Conflicts:
#	.flowconfig
#	app.json
#	flow-typed/npm/expo_vx.x.x.js
#	flow-typed/npm/ramda_vx.x.x.js
#	flow-typed/npm/react-test-renderer_vx.x.x.js
#	package.json
#	yarn.lock
…ure. Merged with the latest version of serlo/master
…icate with parent component (calling changeAnswer) or in this case - redux store
# Conflicts:
#	flow-typed/npm/expo_vx.x.x.js
#	flow-typed/npm/ramda_vx.x.x.js
#	flow-typed/npm/react-native-router-flux_vx.x.x.js
#	flow-typed/npm/react-test-renderer_vx.x.x.js
#	package.json
#	src/actions/exerciseActions.js
#	src/assets/courses/course.json
#	src/components/ExerciseLayout.js
#	src/components/NavigationMenu.js
#	src/components/NavigationMenu.stories.js
#	src/components/exercises/DifferFromSymbol.js
#	src/components/exercises/index.js
#	src/constants/actionTypes.js
#	src/containers/ExerciseApp.js
#	src/containers/ExerciseApp.stories.js
#	src/index.stories.js
#	src/reducers/exercise.js
#	storybook/stories.js
@igorzamyslov
Copy link
Contributor

Hello @inyono,
I have modified this branch a bit (will push it in a moment) - I have changed a couple of exercise-components in the way so that it was possible for them to communicate with parent component/redux store, meaning that now when you click on some answer these components call changeAnswer callback function

The modified exercises are:

  • DifferFromSymbol
  • BuildSentence
  • MissingText (with this one I'm not sure for now how to dynamically change the assets - images/sounds/videos, so it looks really simple)

The full example could be checked in the exercises/Exercise Page story - for now I've added an alert which informs you if the exercise was answered correctly or not

It would be great if you could check these components and see if this approach fits you or maybe you have something different in mind :)

In due time I will check "Schreiben Sie den Buchstaben" exercise one more time. As far as I remember - it's working in principle, but had a low perfomance.

Please write if you will have any questions

@inyono
Copy link
Member Author

inyono commented Sep 18, 2017

Hey @igorzamyslov. Looks good to me, we can work with that 👍. Is there some stuff left to do for this PR, or are you done from your side? (If so, I'd start integrating this into our current master)

@igorzamyslov
Copy link
Contributor

@inyono, I think that most things are already implemented, let's just recap what was done here:

  • Redux router integrated (nothing fancy in the code, just navigation between pages as an example)
  • Redux store integrated with redux-persist module which saves store's state (If you'll decide to work with redux)
  • Course can be imported through JSON file (though here I'm still not sure about the way to dynamically load assets), which contains hierarchy Course -> Sections -> Chapters -> Exercises
  • Created an "Exercise App" which works with the redux store and works in the way that exercises that were answered incorrectly are automatically duplicated in the end of the course
  • To integrate the whole logic a couple of components were changed to be able to work via callback functions from parents

That's what I've remembered :)
In the last commit I've also merged the branch with your master, so I hope it will be easy to integrate.

Please tell if there is anything that should be implemented / changed here.
And we can also discuss the whole thing next week (when Andrej Nikonov will be back), as it will give us a better insight

@inyono
Copy link
Member Author

inyono commented Sep 18, 2017

Hey @igorzamyslov. Looks good from my side. The merge with master isn't pushed yet I guess, since GitHub still shows conflicting files?

@igorzamyslov
Copy link
Contributor

igorzamyslov commented Sep 19, 2017

hey @inyono, I've checked the history - these files were changed after my last push, so I guess that's the reason of the conflicts, but the thing is - after I've merged them just now I can't start the project anymore, I get:
websocket: connection error The operation couldn’t be completed. Connection refused
Same thing happens to me with clean serlo-abc/master branch (with all dependencies updated)

As far as I understand the error occurs on the step, when the app tries to connect to Storybook - any idea why it can be happening?

P.S. this error happens on iOS emulator and the hostname it tries to run Storybook on is localhost, on Android emulator it tries to run Storybook on my local network address (192.168...) but after 10 seconds it gives pretty much the same error

@inyono
Copy link
Member Author

inyono commented Sep 19, 2017

@igorzamyslov Yeah, yarn start doesn't start the storybook anymore. So you need to run yarn storybook additionally in another terminal. (see #128)

# Conflicts:
#	package.json
#	src/components/exercises/DifferFromSymbol.js
#	src/components/exercises/DifferFromSymbol.stories.js
#	src/components/exercises/MissingText.js
#	src/components/exercises/VideoQuestion.js
@igorzamyslov
Copy link
Contributor

igorzamyslov commented Sep 19, 2017

@inyono, yep, that was the step I was missing, thank you!
I've merged 2 branches - everything seems to be working :)

I'll wait for check to finish and will fix anything that will come up

@inyono inyono merged commit c2268a0 into serlo:master Sep 19, 2017
@inyono
Copy link
Member Author

inyono commented Sep 19, 2017

@igorzamyslov Thank you very much for your work 🎉 I'll ping you if we run into any issues, but your solutions looks rather solid to me 👍

inyono pushed a commit that referenced this pull request Sep 19, 2017
@inyono inyono changed the title [WIP] Course rendering course rendering Sep 19, 2017
@igorzamyslov
Copy link
Contributor

Was glad to help!

I'll ping you if we run into any issues

Please do :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants