add ability to refresh RHS details

[manland]

Fixes #131

With part of #132

[marianunez]

Thanks @manland! Could you add a closer screenshot of the new refresh button for UX review?

[marianunez]

@manland thanks for adding that extra part of #132. While testing it out, did you feel it added value to gray out the read item even though you refreshed manually clicking the button?

I believe the original intent of graying out was if you lose focus on the browser and when it returned, it would automatically refresh which could've removed items from the list possibly confusing the user of where they went. Thoughts @lieut-data?

[manland]

@marianunez I think it's a good feature even when I refresh manually. On my gif I switch between github and MM, but in real life, I imagine I will open PR view and stay on it every day (when I have done the dev in gitlab-plugin because I use gitlab... but it's another story :D) and when a coworker, accept/close a MR I have a clear picture of what happened. Like that I will be able to inactive notif /github settings notif off, which is a lot perturbing when I develop.

But IMHO I think adding a label merged or accepted which explain why the item is grey will be a good improvement ;) And when I was playing with the RHS, I want to be able to do action directly in MM (close issue, mark as read notif, or accept PR...)

[aaronrothschild]

This is a great improvement! Thanks @manland for your contribution.

[lieut-data]

Thanks, @manland! Is there an opportunity to extend/introduce unit tests to capture some of this behaviour?

[manland]

@lieut-data thank you very much for this very smart review! I'm sorry for the delay to continue this PR but last week was hot 🔥 (wife work travel... 😄) and I had read lot of blog around react.hooks (i'm angular dev 🤷‍♂️), anyway it was funny!

I'll implem test as soon as possible 👨‍🍳

[manland]

Thanks @manland! Could you add a closer screenshot of the new refresh button for UX review?

@marianunez this is some screenshot of refresh button (default, mouse hover, dark default, dark mouse hover)... sorry for delay explication!

[manland]

I found a bug in my last implementation : if i click refresh 2 times, the second time will remove the item previously marked has missing. Because compo render is called and usePrevious return not previous but last items :

• first call 8 items
• second call (on refresh) 7 items ==> 1 marked has missing
• third call (on refresh again) 7 items ==> compared to previous no missing

To fix it, I can implement the second parameter of React.memo to check if a render is needed depending on items ;) But it will increase the complexity of this component ;)

@lieut-data what do you think about that?

[marianunez]

@marianunez this is some screenshot of refresh button (default, mouse hover, dark default, dark mouse hover)... sorry for delay explication!

Thanks @manland! cc @asaadmahmood for UX review 👆

[asaadmahmood]

@marianunez it should use center-channel-color opacity .6 for its default state, and then center-channel-color opacity .8 for its hovered state.

[asaadmahmood]

See comments.

[lieut-data]

@manland, I admit that I'm not yet well versed enough with React Hooks to describe the right solution in that context.

But as a React.Component, I'd expect to update my state's copy of the items only when refresh is called, and not try to tie it to re-renders due to props changes. Perhaps you could keep the last timestamp the refresh button was pressed (in the sidebar right), and pass that as a prop to the items component? So long as that value hasn't changed, you can keep your "old" props around and use them to render dimmed, etc.

I'll leave it to you to figure out if it makes sense to model that a hook or as a React.Component with old-school state.

[manland]

I'm always on it... now I have test I can try to find a solution :D

@lieut-data i'm not a big fan of this kind of hack, because if user click on refresh in LHS, compo will not be updated on RHS. I continue to play with hooks, and if nothing works I will make a real old school component^^

[lieut-data]

i'm not a big fan of this kind of hack, because if user click on refresh in LHS, compo will not be updated on RHS. I continue to play with hooks, and if nothing works I will make a real old school component

@manland, if I understand hooks correctly, there no more or less powerful than components. Just a cleaner way to write certain patterns. So we'll still need to solve the same problem.

With respect to the refresh, my gut instinct would be to model the timestamp in the Redux store instead of internal state. This would make the the transition across a manual / automatic refreshes and open / close transitions easy to follow.

[manland]

With respect to the refresh, my gut instinct would be to model the timestamp in the Redux store instead of internal state. This would make the the transition across a manual / automatic refreshes and open / close transitions easy to follow.

I'm sorry @lieut-data but I don't understand your solution 😓

I fix the issue by adding an areEqual method in React.memo to be able to not re-render if props are the same.

Just tell me if it's the react way or not ;)

@asaadmahmood css updated 💪 🌠

[asaadmahmood]

@manland Can I get an updated screenshot?

[marianunez]

Overall LGTM, minor comments below. Thanks for optimizing with React.memo and adding the unit tests 👌

[marianunez]

Missing a new line here

[marianunez]

Do we need this? I believe this is turned on by default on all repos

[marianunez]

Nice! 👌

[lieut-data]

Hmm, React.memo is a strategy to boost performance by reducing unnecessary renders, but it seems were relying on it here to help enforce business logic about when to transition "old state" so as to achieve the desired rendering logic.

This approach is still buggy. As an example, if I change my theme, the dimmed items will disappear, and it's really hard to fix this.

I'd like to suggest we move all of this logic into a reducer instead. It should be the reducer's responsibility to decide, based on the incoming actions, when to clear the "old state" and when to preserve it.

[marianunez]

Missing new line here

[lieut-data]

Hmm, React.memo is a strategy to boost performance by reducing unnecessary renders, but it seems were relying on it here to help enforce business logic about when to transition "old state" so as to achieve the desired rendering logic.

This approach is still buggy. As an example, if I change my theme, the dimmed items will disappear, and it's really hard to fix this.

I'd like to suggest we move all of this logic into a reducer instead. It should be the reducer's responsibility to decide, based on the incoming actions, when to clear the "old state" and when to preserve it.

[mattermod]

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

[jasonblais]

@manland let us know if you have any questions about the feedback above. We'd be happy to clarify! :)

[lieut-data]

@manland, would you still be interested in contributing towards this pull request? Let me know if there's any clarification I can help with :)

[manland]

@lieut-data oups sorry this issue was out of my radar 🤷‍♂️

Yes I want to finish it, but I have no idea how to implement a good solution. Could you, please, give me more details on your proposed implementation?

[lieut-data]

Absolutely! I don't yet have enough experience to frame this as a React hook, but let me just talk about fundamentals.

To render some items as "dimmed", we effectively need to remember the set of items we loaded when the RHS was first opened or when it was last manually refreshed, in addition to the set of items we've received via a websocket event.

Let's take mentions as a concrete example. We already model these in the Redux reducer:

function mentions(state = [], action) {
    switch (action.type) {
    case ActionTypes.RECEIVED_MENTIONS:
        return action.data;
    default:
        return state;
    }
}

We should start by modifying the RECEIVED_MENTIONS handling to merge the old state with the incoming action.data. If we find an item in the old state that's not in the new action.data, we can add a boolean, say stale. The client-side can directly use this bit to decide whether to render the item as dimmed. In pseudo-code, something like:

    case ActionTypes.RECEIVED_MENTIONS:
        // clone state (can't mutate in place)
        // iterate through items in action.data
        //     if item.id is not in state, set state[item.id].stale = true
        //     otherwise overwrite with state[item.id] = action.data[item.id]
        // return cloned state
}

For the initial load event -- i.e. opening the RHS -- we can probably just purge the entire state -- stale and all -- and wait for the incoming mentions. When manually refreshing, we want to both get rid of the stale mentions and replace with the new mentions. The easiest way to do that is to add a new action that also makes the same HTTP request to GitHub, but gets handled the old way:

    case ActionTypes.REFRESHED_MENTIONS:
        return action.data;

I'm sure there's different ways to slice this, but the key take away is to model the desired state in Redux, use actions to transform the data based on events, and avoid tying up too much of this business logic into the component itself.

[hanzei]

Hi @manland,

Given that this PR has been stale for a long time and I don't expect an update happening in the near future, I will close it for now. Please let me know, if you want to continue your work.

[manland]

Hey @hanzei yes thank you... I really want to... But I can't... If someone want to finish it, I have done 80% of the dev, the last thing it's to move the logic from component to reducer requested by @lieut-data