Skip to content

Fix count values on platform selection on the home page#781

Merged
joanagmaia merged 5 commits intolinuxfoundation:mainfrom
peoray:total-number-platform-selection
Apr 24, 2023
Merged

Fix count values on platform selection on the home page#781
joanagmaia merged 5 commits intolinuxfoundation:mainfrom
peoray:total-number-platform-selection

Conversation

@peoray
Copy link
Copy Markdown
Contributor

@peoray peoray commented Apr 18, 2023

Changes proposed ✍️

Fix count values on platform selection on the home page

What

🤖 Generated by Copilot at b43a433

/claim #752

Added platform filter and improved loading indicator for dashboard widget header. The filter allows users to see platform-specific data, and the indicator shows the correct loading state for the total counts.
Fixes #752

🤖 Generated by Copilot at b43a433

To show counts of members and acts
We added a platform filter fact
But the app-loading prop
Needed a little swap
To match the total data impact

Why

How

🤖 Generated by Copilot at b43a433

  • Use totalLoading prop instead of loading prop for app-loading component in dashboard widget header to match total counts of activities and members (link)
  • Pass platform filter from state to ActivityService.list and MemberService.list methods in dashboard store actions to get platform-specific counts of activities and members for dashboard widget header (link, link)

Checklist ✅

  • Label appropriately with Feature, Improvement, or Bug.
  • Add screehshots to the PR description for relevant FE changes
  • New backend functionality has been unit-tested.
  • API documentation has been updated (if necessary) (see docs on API documentation).
  • Quality standards are met.

Copy link
Copy Markdown
Contributor

@joanagmaia joanagmaia left a comment

Choose a reason for hiding this comment

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

Once again thanks so much for the contribution 😄
Almost there! Just a couple remarks:

  • Please fix eslint related issues. You can see that one of the build failed in PR
  • What you updated in the actions.js file is good 👌 But I believe you are also missing for Conversation and Organizations. So the same way you updated for getActivitiesCount and getMembersCount, you should also update for getConversationCount, and getOrganizationsCount

@joanagmaia joanagmaia added the Bug Created by Linear-GitHub Sync label Apr 21, 2023
@peoray
Copy link
Copy Markdown
Contributor Author

peoray commented Apr 21, 2023

@joanagmaia thanks for the feedback.
Regarding conversation and organization, the issue only mentioned members and activities, hence why I worked on them only.

@joanagmaia joanagmaia added the Contributor Created by Linear-GitHub Sync label Apr 21, 2023
@joanagmaia
Copy link
Copy Markdown
Contributor

joanagmaia commented Apr 21, 2023

@joanagmaia thanks for the feedback. Regarding conversation and organization, the issue only mentioned members and activities, hence why I worked on them only.

You are right, it should have been better detailed, apologies for that. I'll take care of updating the issue description to make sure that it also contains for organizations and conversations.
All 4 should respond in the same way to the filters applied so I really think we should update for all of them at once
@jonathimer the issue is happening for all dashboard widgets right?

@peoray
Copy link
Copy Markdown
Contributor Author

peoray commented Apr 21, 2023

@joanagmaia I'm not sure how to use the conversation. Can you help? There's no visual representation

Copy link
Copy Markdown
Contributor

@joanagmaia joanagmaia left a comment

Choose a reason for hiding this comment

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

@peoray you are right Conversation does not have a visual representation, that's why it was commented. I missed that one, sorry about the confusion.
Could you undo the changes for Conversations? Other than that looks good to me for Members, Organizations and Activities. Once you undo for Conversations I'll approve it

@peoray
Copy link
Copy Markdown
Contributor Author

peoray commented Apr 21, 2023

@joanagmaia Resolved

Copy link
Copy Markdown
Contributor

@joanagmaia joanagmaia left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@joanagmaia joanagmaia merged commit 71f98dd into linuxfoundation:main Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Created by Linear-GitHub Sync Contributor Created by Linear-GitHub Sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C-860] "Total numbers" on Home don't get affected by platform selection

2 participants