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

Update interesting people sidebar block #857

Merged
merged 13 commits into from Oct 27, 2017

Conversation

Projects
None yet
4 participants
@jm90m
Member

jm90m commented Oct 19, 2017

from trello:

Use the steem.api.getBlogAuthorsAsync(username) and load the top most rebloged users, then do steem.api.getAccountsAsync(['username1', username2', 'username3']) 
to load the 3 accounts details. Load the sidebar right box "Interesting People" 
with user metadata.name and metadata.about. 
Dont show this box if user has less than 3 top rebloged author.

Test get blog authors request: https://api.steemjs.com/get_blog_authors?blogAccount=fabien

@jm90m jm90m requested a review from bonustrack Oct 19, 2017

@bonustrack bonustrack temporarily deployed to busy-dev-pr-857 Oct 19, 2017 Inactive

@bonustrack

This comment has been minimized.

Show comment
Hide comment
@bonustrack

bonustrack Oct 20, 2017

Member
  • Can we render only username (dont need to call getAccounts to get description) with a limit of 5?
  • This PR should change interesting people from user profile page, we should not change interesting people box on others pages. You can create 2 differents components. One called "Intersting People" that we have already in current busy version with refresh icon. And another one "Recommended" that you are doing now.
Member

bonustrack commented Oct 20, 2017

  • Can we render only username (dont need to call getAccounts to get description) with a limit of 5?
  • This PR should change interesting people from user profile page, we should not change interesting people box on others pages. You can create 2 differents components. One called "Intersting People" that we have already in current busy version with refresh icon. And another one "Recommended" that you are doing now.

@bonustrack bonustrack temporarily deployed to busy-dev-pr-857 Oct 20, 2017 Inactive

@bonustrack bonustrack temporarily deployed to busy-dev-pr-857 Oct 20, 2017 Inactive

@bonustrack

This comment has been minimized.

Show comment
Hide comment
@bonustrack

bonustrack Oct 20, 2017

Member
  • Layout of interesting people should be same before:
    image
  • Also the request is sent 2 times if you are logged and open a user profile from new tab
    image
Member

bonustrack commented Oct 20, 2017

  • Layout of interesting people should be same before:
    image
  • Also the request is sent 2 times if you are logged and open a user profile from new tab
    image

@bonustrack bonustrack added the feature label Oct 22, 2017

@bonustrack bonustrack added this to In Progress 🏗 in Workflow Oct 24, 2017

@bonustrack bonustrack temporarily deployed to busy-dev-pr-857 Oct 25, 2017 Inactive

@bonustrack bonustrack temporarily deployed to busy-dev-pr-857 Oct 25, 2017 Inactive

@bonustrack bonustrack temporarily deployed to busy-dev-pr-857 Oct 26, 2017 Inactive

@bonustrack bonustrack temporarily deployed to busy-dev-pr-857 Oct 26, 2017 Inactive

@Sekhmet

This comment has been minimized.

Show comment
Hide comment
@Sekhmet

Sekhmet Oct 26, 2017

Member

Make sure not to lose changes made in #893 when merging.

Member

Sekhmet commented Oct 26, 2017

Make sure not to lose changes made in #893 when merging.

@bonustrack bonustrack temporarily deployed to busy-dev-pr-857 Oct 27, 2017 Inactive

@jm90m

This comment has been minimized.

Show comment
Hide comment
@jm90m

jm90m Oct 27, 2017

Member

@bonustrack @Sekhmet Just merged and got latest changes from new-design (as in #893)

This should fix: #904

Member

jm90m commented Oct 27, 2017

@bonustrack @Sekhmet Just merged and got latest changes from new-design (as in #893)

This should fix: #904

@ryanbaer

This comment has been minimized.

Show comment
Hide comment
@ryanbaer

ryanbaer Oct 27, 2017

Contributor

Quick question - should you be making a network call (i.e. steemAPI.getBlogAuthorsAsync) from within a component? The typical approach I've seen in this app, since we're using Redux, is to dispatch actions that make use of the Redux state tree to feed in data.

I'm newer to the codebase so maybe there's something I'm missing, but that stands out to me. user/userActions.js is a good example that also makes use of steemAPI but with this approach.

Contributor

ryanbaer commented Oct 27, 2017

Quick question - should you be making a network call (i.e. steemAPI.getBlogAuthorsAsync) from within a component? The typical approach I've seen in this app, since we're using Redux, is to dispatch actions that make use of the Redux state tree to feed in data.

I'm newer to the codebase so maybe there's something I'm missing, but that stands out to me. user/userActions.js is a good example that also makes use of steemAPI but with this approach.

@ryanbaer

This comment has been minimized.

Show comment
Hide comment
@ryanbaer

ryanbaer Oct 27, 2017

Contributor

Actually a little more to say here:

  • Could you please update the description with a summary of what's in the Trello card? Those of us not on Trello won't have full context for this issue

  • This feature likely needs the same treatment I just gave to the without-api version of Interesting People - don't display people in the list that the user already follows. I'm checking out the app on your branch for someone I follow and it looks like this:
    image

  • Following up on above, the first place I viewed this feature was on my own page and, as you'd expect, everyone in that list is someone that I follow. I think we should display the original Interesting People module on the user's own page.

Contributor

ryanbaer commented Oct 27, 2017

Actually a little more to say here:

  • Could you please update the description with a summary of what's in the Trello card? Those of us not on Trello won't have full context for this issue

  • This feature likely needs the same treatment I just gave to the without-api version of Interesting People - don't display people in the list that the user already follows. I'm checking out the app on your branch for someone I follow and it looks like this:
    image

  • Following up on above, the first place I viewed this feature was on my own page and, as you'd expect, everyone in that list is someone that I follow. I think we should display the original Interesting People module on the user's own page.

@jm90m

This comment has been minimized.

Show comment
Hide comment
@jm90m

jm90m Oct 27, 2017

Member

@ryanbaer good question, I figured since this is a standalone component, and can be separated away from the entire app state. I think it is fine to make API calls within the component. It might be a heavy load to go through the reducers just for a simple block component. The same thing is done in PostRecommendation component

Member

jm90m commented Oct 27, 2017

@ryanbaer good question, I figured since this is a standalone component, and can be separated away from the entire app state. I think it is fine to make API calls within the component. It might be a heavy load to go through the reducers just for a simple block component. The same thing is done in PostRecommendation component

@jm90m

This comment has been minimized.

Show comment
Hide comment
@jm90m

jm90m Oct 27, 2017

Member

@ryanbaer

  • updated description with the contents of trello card
  • good point, with displaying the original InterestingPeople component on the users own page, I'll add it in
Member

jm90m commented Oct 27, 2017

@ryanbaer

  • updated description with the contents of trello card
  • good point, with displaying the original InterestingPeople component on the users own page, I'll add it in

@bonustrack bonustrack temporarily deployed to busy-dev-pr-857 Oct 27, 2017 Inactive

@ryanbaer

This comment has been minimized.

Show comment
Hide comment
@ryanbaer

ryanbaer Oct 27, 2017

Contributor

Thanks for those updates - much appreciated.

I've thought about and done some reading on the network request from the component. Personally, I feel that since the app primarily functions with Redux, we should keep all application state in the state tree. This was also the general impression I got from reading issues on the redux repo (i.e. Question: How to choose between Redux's store and React's state? )

The co-creator of redux (Dan Abramov) said the following:

Use React for ephemeral state that doesn't matter to the app globally and doesn't mutate in complex ways. For example, a toggle in some UI element, a form input state. Use Redux for state that matters globally or is mutated in complex ways. For example, cached users, or a post draft.

It's not a strict rule. There are always exceptions, but in our case, I'd consider the interesting posts and and interesting users to be application state rather than ephemeral UI state.

And either way, from the perspective of contributors, I think it's important we clarify when state should be managed by Redux versus dropping it down into the individual components.

I know it is a little more work to setup action creators, etc., but I personally think we should stick to the paradigm that's present throughout the rest of the app.

I'd like to hear from others on this as well, like @bonustrack and @Sekhmet.

I'm open to the idea of managing state in components if there's a solid argument for it, but I think it'd be good to setup guidelines on when each approach should be applied.


As an aside, I also wanted to say I'm looking forward to this feature being merged in. I think it helps to enhance the network effect on Busy by allowing you to be introduced to "Related People" in a sense. Nice work on getting it in.

Contributor

ryanbaer commented Oct 27, 2017

Thanks for those updates - much appreciated.

I've thought about and done some reading on the network request from the component. Personally, I feel that since the app primarily functions with Redux, we should keep all application state in the state tree. This was also the general impression I got from reading issues on the redux repo (i.e. Question: How to choose between Redux's store and React's state? )

The co-creator of redux (Dan Abramov) said the following:

Use React for ephemeral state that doesn't matter to the app globally and doesn't mutate in complex ways. For example, a toggle in some UI element, a form input state. Use Redux for state that matters globally or is mutated in complex ways. For example, cached users, or a post draft.

It's not a strict rule. There are always exceptions, but in our case, I'd consider the interesting posts and and interesting users to be application state rather than ephemeral UI state.

And either way, from the perspective of contributors, I think it's important we clarify when state should be managed by Redux versus dropping it down into the individual components.

I know it is a little more work to setup action creators, etc., but I personally think we should stick to the paradigm that's present throughout the rest of the app.

I'd like to hear from others on this as well, like @bonustrack and @Sekhmet.

I'm open to the idea of managing state in components if there's a solid argument for it, but I think it'd be good to setup guidelines on when each approach should be applied.


As an aside, I also wanted to say I'm looking forward to this feature being merged in. I think it helps to enhance the network effect on Busy by allowing you to be introduced to "Related People" in a sense. Nice work on getting it in.

@bonustrack bonustrack temporarily deployed to busy-dev-pr-857 Oct 27, 2017 Inactive

@bonustrack

This comment has been minimized.

Show comment
Hide comment
@bonustrack

bonustrack Oct 27, 2017

Member

When page is loading i will see this:
image
It feel like the loading is complete and little bit confusing. There should be either a loading or the component completely loaded with users visible but not empty component.

Member

bonustrack commented Oct 27, 2017

When page is loading i will see this:
image
It feel like the loading is complete and little bit confusing. There should be either a loading or the component completely loaded with users visible but not empty component.

@jm90m

This comment has been minimized.

Show comment
Hide comment
@jm90m

jm90m Oct 27, 2017

Member

@ryanbaer You make a valid point, but the reason why I think interesting posts and interesting people should be in its own React state is that the data for them is always rapidly changing, when navigating between the different users.

Heres a few questions when determining what kind of data should be put into Redux (taken from https://spin.atomicobject.com/2017/06/07/react-state-vs-redux-state/):

  1. Do other parts of the application care about this data?
  2. Do you need to be able to create further derived data based on this original data?
  3. Is the same data being used to drive multiple components?
  4. Is there value to you in being able to restore this state to a given point in time (ie, time travel debugging)?
  5. Do you want to cache the data (ie, use what’s in state if it’s already there instead of re-requesting it)?

For this specific component, here is how I would answer these questions:

  1. No the application is completely separated from this component, the InterestingPeopleWithAPI component is dependent to each /user page. The original InterestingPeople is not specific to each /user page, it's more specific to the current user, that's why it should be in the redux state
  2. Nope
  3. Nope
  4. Maybe, for caching the data, so you don't have to make another request, just check to see if data is already present
  5. The data set is specific to each /user page, so caching it might be good, but the data might be changing since we want to get the top reblogged users for each user.
Member

jm90m commented Oct 27, 2017

@ryanbaer You make a valid point, but the reason why I think interesting posts and interesting people should be in its own React state is that the data for them is always rapidly changing, when navigating between the different users.

Heres a few questions when determining what kind of data should be put into Redux (taken from https://spin.atomicobject.com/2017/06/07/react-state-vs-redux-state/):

  1. Do other parts of the application care about this data?
  2. Do you need to be able to create further derived data based on this original data?
  3. Is the same data being used to drive multiple components?
  4. Is there value to you in being able to restore this state to a given point in time (ie, time travel debugging)?
  5. Do you want to cache the data (ie, use what’s in state if it’s already there instead of re-requesting it)?

For this specific component, here is how I would answer these questions:

  1. No the application is completely separated from this component, the InterestingPeopleWithAPI component is dependent to each /user page. The original InterestingPeople is not specific to each /user page, it's more specific to the current user, that's why it should be in the redux state
  2. Nope
  3. Nope
  4. Maybe, for caching the data, so you don't have to make another request, just check to see if data is already present
  5. The data set is specific to each /user page, so caching it might be good, but the data might be changing since we want to get the top reblogged users for each user.

@bonustrack bonustrack temporarily deployed to busy-dev-pr-857 Oct 27, 2017 Inactive

@bonustrack

This comment has been minimized.

Show comment
Hide comment
@bonustrack

bonustrack Oct 27, 2017

Member

When i open the page i can see the right sidebar empty for few seconds. There should be a loading instead:
image

Member

bonustrack commented Oct 27, 2017

When i open the page i can see the right sidebar empty for few seconds. There should be a loading instead:
image

@bonustrack bonustrack temporarily deployed to busy-dev-pr-857 Oct 27, 2017 Inactive

@ryanbaer

This comment has been minimized.

Show comment
Hide comment
@ryanbaer

ryanbaer Oct 27, 2017

Contributor

All fair points @jm90m, thanks for looking into it as well. You're helping me to see what's about preferences here - I really prefer lifting state into a single location in the Redux store, and having components be dumb (i.e. decoupled from the logic of how the data is produced). Tell me if this isn't right, but it sounds like you prefer to keep components isolated to their concerns when it's possible, i.e. the only place we're currently making use of interesting people and interesting posts is in the respective components.

Another question on this PR: now that I'm looking at your network call in the component - should your call have a .catch to respond to any possible errors on your request as well? And maybe that simply prompts the noUsers: true that you set in other cases. In this case. Alternatively, I guess you could avoid doing a catch handler if you set noUsers default to true, but that would change your logic around a bit.

Also I saw that you added a filter for already followed users, which is awesome. When I suggested it, it didn't occur to me that accomplishing that means we have to forfeit the limit on the network request, effectively always retrieving the user's entire blogAuthors list to increase the probability that we can find 5 users not already followed. If that list is incredibly long for some users, it could take a while to load, but I think feedback from the community will be a good driver of tackling that issue.

Thanks for the discussion and the great PR. Excited to have this feature live.

Contributor

ryanbaer commented Oct 27, 2017

All fair points @jm90m, thanks for looking into it as well. You're helping me to see what's about preferences here - I really prefer lifting state into a single location in the Redux store, and having components be dumb (i.e. decoupled from the logic of how the data is produced). Tell me if this isn't right, but it sounds like you prefer to keep components isolated to their concerns when it's possible, i.e. the only place we're currently making use of interesting people and interesting posts is in the respective components.

Another question on this PR: now that I'm looking at your network call in the component - should your call have a .catch to respond to any possible errors on your request as well? And maybe that simply prompts the noUsers: true that you set in other cases. In this case. Alternatively, I guess you could avoid doing a catch handler if you set noUsers default to true, but that would change your logic around a bit.

Also I saw that you added a filter for already followed users, which is awesome. When I suggested it, it didn't occur to me that accomplishing that means we have to forfeit the limit on the network request, effectively always retrieving the user's entire blogAuthors list to increase the probability that we can find 5 users not already followed. If that list is incredibly long for some users, it could take a while to load, but I think feedback from the community will be a good driver of tackling that issue.

Thanks for the discussion and the great PR. Excited to have this feature live.

@jm90m

This comment has been minimized.

Show comment
Hide comment
@jm90m

jm90m Oct 27, 2017

Member

@ryanbaer yeah I think it's all a matter of preference, but I do prefer to keep components isolated within themselves, if they don't affect the entire app. But I'm actually looking into refactoring this PR, to try to move it over to the state (I will test to see which is more performant) and just use one component for InterestingPeople. Good point on the .catch clause, I will make sure to add that whether I decide to keep it within the component / move it over to redux. Thanks for your feedback :)

Member

jm90m commented Oct 27, 2017

@ryanbaer yeah I think it's all a matter of preference, but I do prefer to keep components isolated within themselves, if they don't affect the entire app. But I'm actually looking into refactoring this PR, to try to move it over to the state (I will test to see which is more performant) and just use one component for InterestingPeople. Good point on the .catch clause, I will make sure to add that whether I decide to keep it within the component / move it over to redux. Thanks for your feedback :)

@jm90m

This comment has been minimized.

Show comment
Hide comment
@jm90m

jm90m Oct 27, 2017

Member

@ryanbaer Actually I've been trying to refactor it for the past few hrs, I think having theblogAuthors in the redux state will just overcomplicate it. Going to keep it 2 components for now. And see what other people think.

Member

jm90m commented Oct 27, 2017

@ryanbaer Actually I've been trying to refactor it for the past few hrs, I think having theblogAuthors in the redux state will just overcomplicate it. Going to keep it 2 components for now. And see what other people think.

@bonustrack bonustrack temporarily deployed to busy-dev-pr-857 Oct 27, 2017 Inactive

@bonustrack

LGTM

@bonustrack bonustrack requested a review from Sekhmet Oct 27, 2017

@jm90m jm90m merged commit 26231b9 into new-design Oct 27, 2017

2 checks passed

Better Code Hub ✅ Better Code Hub approves this code
Details
ci/circleci Your tests passed on CircleCI!
Details

@jm90m jm90m deleted the nd/update-interesting-people-sidebar-block branch Oct 27, 2017

@bonustrack bonustrack moved this from In Progress 🏗 to Shipped 🚀 in Workflow Oct 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment