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

Hide the table when data is loading #11238

Merged

Conversation

muditchoudhary
Copy link
Contributor

Summary

Hides the table when this.dataLoading is true in coreTable.vue

kolibri

References

#10993

Reviewer guidance

Visit the Group section under plan under Coach in Kolibri. Throttle the network to slow 3g.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@MisRob MisRob requested review from MisRob and pcenov September 15, 2023 05:43
@MisRob
Copy link
Member

MisRob commented Sep 15, 2023

Thanks @muditchoudhary!

@pcenov Could you please preview several random tables in Kolibri and see if you can spot any regressions of loading states and displaying of table data afterwards?

@MisRob MisRob added the TODO: needs review Waiting for review label Sep 15, 2023
Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

Code is looking good to me. Let's wait for QA results before merging.

@pcenov
Copy link
Member

pcenov commented Sep 15, 2023

Hi @MisRob this is implemented as specified and I didn't observe any regressions. Just a clarifying question - is this change supposed to be made for other tables as well such as the Groups table under Coach > Reports because now the tables are hidden while loading only at Coach > Plan?

@MisRob
Copy link
Member

MisRob commented Sep 15, 2023

@pcenov This particular change addresses just some edge cases - it ensures that data are not displayed simultaneously with a loader and it will affect all tables in Kolibri.

@MisRob
Copy link
Member

MisRob commented Sep 15, 2023

Just a clarifying question - is this change supposed to be made for other tables as well such as the Groups table under Coach > Reports because now the tables are hidden while loading only at Coach > Plan?

@pcenov This sounds like one of the existing inconsistencies in loading states in Coach. I assume it's present before this PR?

@pcenov
Copy link
Member

pcenov commented Sep 15, 2023

@MisRob that's precisely what I mean - the table issues at Coach > Reports are preexisting so I was wondering whether they will be fixed with a separate PR.

@MisRob
Copy link
Member

MisRob commented Sep 15, 2023

@pcenov I think so, but if you could upload a recording, that'd be helpful so I know exactly what problems you mean. We can then transfer it to a new issue if needed.

@pcenov
Copy link
Member

pcenov commented Sep 15, 2023

Yeah, sure - here's the video comparing what I see while I am at Coach > Plan vs Coach > Reports:

2023-09-15_11-26-51.mp4

@MisRob
Copy link
Member

MisRob commented Sep 15, 2023

Ah yes, thank you. I think that's one of the consequences of #11219 and not directly to work in this PR.

@MisRob
Copy link
Member

MisRob commented Sep 15, 2023

I am linking it to #11219

Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Thank you @MisRob, in that case I am going to approve this one and we can track the other changes separately.

@MisRob MisRob merged commit bed1128 into learningequality:develop Sep 15, 2023
@muditchoudhary muditchoudhary deleted the issue-10993-hide-table branch September 17, 2023 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants