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

Create common components to represent dashboard activity pages #6291

Merged
merged 1 commit into from
May 28, 2024

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented May 24, 2024

Depends on #6285

Create a common OrderableActivityTable component to render activity tables in the dashboard pages. This allows to:

  1. Test the table ordering logic only once.
  2. Have a consistent structure and look-and-feel between sections.

image

image

Additionally, this PR makes these other changes:

  1. Render titles using CardHeader + CardTitle instead of a h2 inside the CardContent itself. This title will potentially be removed replaced by a breadcrumb, but that requires a bit of experimentation.
  2. Make all columns in the table but the first one be sorted in descending order by default, thanks to the implementation from Allow to provide initial order per DataTable column frontend-shared#1564

@acelaya acelaya force-pushed the dashboard-consolidations branch 3 times, most recently from 13d2da5 to 1672dd2 Compare May 27, 2024 09:38
@acelaya acelaya force-pushed the dashboard-course-stats branch from 9150c5e to 7d3abd6 Compare May 27, 2024 11:48
Base automatically changed from dashboard-course-stats to main May 27, 2024 11:53
@acelaya acelaya force-pushed the dashboard-consolidations branch 3 times, most recently from aa44c92 to 093157c Compare May 27, 2024 14:25
@acelaya acelaya force-pushed the dashboard-consolidations branch from 093157c to 7442f4f Compare May 27, 2024 14:35
@acelaya acelaya marked this pull request as ready for review May 27, 2024 14:45
@acelaya acelaya requested a review from robertknight May 27, 2024 14:57
@acelaya acelaya force-pushed the dashboard-consolidations branch from 7442f4f to ed63b68 Compare May 27, 2024 15:46
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

This generally looks good. Just some minor comments.

<CardHeader fullWidth>
<CardTitle tagName="h2">
<span data-testid="title">
{course.isLoading && 'Loading...'}
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to the changes in this PR, when transitioning from a course to an assignment, we already know the assignment title so we could use that as the initial value until we get the "latest" value back from the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, we should do that.

I have been lately thinking on how we can better handle transitions between routes, and then combine that with a global loading state/splash screen for the initial load. I'll be creating some POC soon.

>((acc, columnName) => {
// All stat columns should be ordered in descending order first
acc[columnName] =
typeof columnName === 'string' && statsColumns.includes(columnName)
Copy link
Member

Choose a reason for hiding this comment

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

What is the typeof for here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

columnName has type keyof T, which translates into string | number | symbol.

Without the check for string type, TypeScript complains when passed to Array<string>.includes().

This is related to the discussion here https://hypothes-is.slack.com/archives/C1M8NH76X/p1716819580379869, but I skipped this part from the conversation because it was easy to solve.

@acelaya acelaya force-pushed the dashboard-consolidations branch from ed63b68 to 0508acc Compare May 28, 2024 06:41
@acelaya acelaya requested a review from robertknight May 28, 2024 06:41
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple of notes about comments.

@acelaya acelaya force-pushed the dashboard-consolidations branch from 0508acc to 65f5ea4 Compare May 28, 2024 07:45
@acelaya acelaya merged commit 4b1575c into main May 28, 2024
8 checks passed
@acelaya acelaya deleted the dashboard-consolidations branch May 28, 2024 08:07
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.

2 participants