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

Clear w/l display on player page switch #3150

Merged
merged 4 commits into from
Feb 7, 2024

Conversation

jeyren95
Copy link
Contributor

@jeyren95 jeyren95 commented Feb 7, 2024

Cause of issue:

  • the "getPlayerWinLoss" API is being called while navigating between player profiles
  • however, the "playerWinLoss.loading" state is not being checked while rendering the components

Proposed changes:

  • added a check for "playerWinLoss.loading"
  • placed the above check inside "PlayerHeader" component because the component relies on data sets from 2 individual APIs
  • this is to prevent different portions of the component from displaying the desired content at different speeds

fixes #3148

@howardchung
Copy link
Member

I think we can achieve this without changing to link elements. Note that when navigating player profile to player profile, the matches, heroes, and peers show a loader without a page refresh. We want to achieve the same thing for the W/L number.

@jeyren95
Copy link
Contributor Author

jeyren95 commented Feb 7, 2024

i see, ok after digging deeper, i managed to find the cause of the issue, and a (probably) better solution. Updated the PR! Please help to take a look, thanks Howard!

@@ -1,4 +1,5 @@
import React from 'react';
import { withRouter } from 'react-router-dom';
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this file needs to be modified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad, forgot to remove while testing some stuff, thanks for spotting

@howardchung
Copy link
Member

Thanks! This is definitely better--ideally we would only show a separate loading state for the win loss section since that tends to load a little slower than the profile info, but that's not required

@jeyren95
Copy link
Contributor Author

jeyren95 commented Feb 7, 2024

hmmm yeah, actually if you want to show a separate loading state for the win loss section, i could just remove the extra condition in the "PlayerHeader" component.

But my initial thought was that it would be more UI-friendly to load the entire component together rather than seeing portions of it loading separately.

What dyou think?

@howardchung
Copy link
Member

I think we should show a loader only for the w/l data. If the name/picture etc. are available first there's no reason to block on it.

@jeyren95
Copy link
Contributor Author

jeyren95 commented Feb 7, 2024

ok done, have removed the extra condition

@howardchung
Copy link
Member

Looks good! I guess we can leave for a future item improving the loader apperance (maybe a shimmer instead of a spinner) and left align it.

@howardchung howardchung merged commit 7b40abd into odota:master Feb 7, 2024
4 checks passed
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.

Clear w/l display on player page switch
2 participants