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

Milestone #2 #7

Open
vit-svoboda opened this issue Nov 16, 2018 · 1 comment
Open

Milestone #2 #7

vit-svoboda opened this issue Nov 16, 2018 · 1 comment

Comments

@vit-svoboda
Copy link

I'm afraid there must've been a misunderstanding in regards to the second milestone. I appreciate the extra effort to implement the back-end calls. However, a lot more features should've been implemented by now, albeit only locally in the browser. I can add a channel and once you're done with API authentication, the channel will be really added. But I can't switch channels, I can't add messages to the channels (and it would've been absolutely fine if the messages disappeared after browser refresh), I can't delete or up/downvote messages etc.

Code suggestions

  • When you dispatch a simple action inside of a thunk action, you have the action definition inline in the body of the thunk action. I'd prefer if there was an action creator function for all of these actions, see add channel vs. add channel failed.
  • Your Redux actions are semanticaly structured in a very imperative way. The action in Redux is just an event (e. g. CHANNEL_ADDITION_STARTED, CHANNEL_ADDITION_FAILED) and the reducers decide how (and whether) to modify the application state when the event happened. Of course, the actions carry payload that the reducers may need to react to the event, but should not dictate the change in any way.
  • You can leverage desctructuring to avoid some of the boring monotonous code either this way if the objects are completly same:
    const mapStateToProps = (state: IRootState): IChatProps => {
    const { auth } = state.chat;
      return {
        ...auth,
      };
    };
    
    Or if you want to be explicit:
    const mapStateToProps = (state: IRootState): IChatProps => {
      const { isLoading, error, content } = state.chat.auth;
      return {
        isLoading,
        error,
        content,
       };
    };
    
    You can use reselect to do that, but it's optional unless you create new objects in this state cherry-picking process. That creates the need some form of memoization. Anyway, same principle applies to the selector body, don't make it too boring to read. 😄
@ondrejvelisek
Copy link
Owner

Honestly, I did not read the milestone at all :/ We will catch up. :)

We discuss naming and structure of actions at the lecture. Your colleagues were not able to clarify what is the desired state.

By ADD_CHANNEL I meant to describe what action user is willing to do. (From Redux doc: ADD_TODO).
Does your suggestion apply to only thunk actions? If so, I think action creator should be still named addChannel(...) since it is natural to call addChannel(...) function in a component. (I expect the use of mapDispatchToProps shorthand).

Thank you for clarification.

PS. Klidne muzeme prepnout do cestiny. ;-)

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

No branches or pull requests

2 participants