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 dashboard organization home page #6325

Merged
merged 1 commit into from
Jun 4, 2024
Merged

Create dashboard organization home page #6325

merged 1 commit into from
Jun 4, 2024

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Jun 3, 2024

Add an organization home page where we list the courses that belong to the organization.

Note

The BE does not currently return the amount of assignments per course. We'll add that in a follow-up PR.

Additionally, I took the opportunity to update to the latest frontend-shared patch release, which includes a fix to allow interacting with links inside DataTables via Enter key.

dashboard-home-2024-06-04_10.48.17.mp4

Testing steps

  1. Open an assignment: https://hypothesis.instructure.com/courses/125/assignments/873
  2. Wait for the sidebar to load, then click on the user dropdown and select "Open dashboard".
  3. The dashboard's assignment view should be loaded, and the breadcrumb should include "Home" as the first item.
  4. Clicking on it should take you to the list of courses that belong to the organization and the instructor is part of.

@acelaya acelaya force-pushed the dashboard-org-page branch from a6a05a9 to 3d3cf1b Compare June 3, 2024 14:53
Comment on lines +10 to +12
const { organizationPublicId } = useParams<{
organizationPublicId: string;
}>();
Copy link
Contributor Author

@acelaya acelaya Jun 3, 2024

Choose a reason for hiding this comment

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

useParams() does not return matching params from parent routes when used inside a nested route, so we need to call this here to get the organization ID, and then pass it down as a regular prop to <OrganizationActivity />.

Copy link
Contributor Author

@acelaya acelaya Jun 4, 2024

Choose a reason for hiding this comment

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

Maybe the parent route should just be /dashboard, and we should add the /organizations/:organizationPublicId in all routes here, but I think we can delay that decision for now..

@acelaya acelaya force-pushed the dashboard-org-page branch from 3d3cf1b to 92e7a4e Compare June 4, 2024 08:36
@@ -47,11 +52,11 @@ export default function AssignmentActivity() {
/>
</div>
)}
<h2 data-testid="title" className="text-lg text-brand font-semibold">
<CardTitle tagName="h2" data-testid="title">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved away from CardTitle on a previous PR, but it was actually not needed, so going back to it to have implicit styles.

@acelaya acelaya force-pushed the dashboard-org-page branch 2 times, most recently from f4b3df5 to 3499120 Compare June 4, 2024 08:39
@@ -32,7 +30,7 @@ const descendingOrderColumns: readonly string[] = [
* Annotation activity table for dashboard views. Includes built-in support for
* sorting columns.
*/
export default function OrderableActivityTable<T extends BaseDashboardStats>({
Copy link
Contributor Author

@acelaya acelaya Jun 4, 2024

Choose a reason for hiding this comment

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

There was actually no reason for this restriction. Removing it allows for more flexibility when rendering orderable tables.

@acelaya acelaya force-pushed the dashboard-org-page branch from 3499120 to dd1e748 Compare June 4, 2024 08:45
@acelaya acelaya marked this pull request as ready for review June 4, 2024 08:52
@acelaya acelaya force-pushed the dashboard-org-page branch from dd1e748 to c6cf4f7 Compare June 4, 2024 08:57
@acelaya acelaya requested a review from robertknight June 4, 2024 09:58
@acelaya acelaya force-pushed the dashboard-org-page branch from c6cf4f7 to 62a225e Compare June 4, 2024 10:29
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, with a recommendation regarding table headers. I suspect there will be opportunities to factor out more parts of the different views into shared components, but the best place to draw the line will become clearer when this app is developed a bit further.

import OrderableActivityTable from './OrderableActivityTable';

export type OrganizationActivityProps = {
organizationPublicId: string;
Copy link
Member

Choose a reason for hiding this comment

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

Do we ever receive anything other than the "public" ID for an organization in the frontend? If not, we could use the term organizationId and make it always refer to the "public" ID.

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, I thought about that, but since the terminology is going to change to organization_public_id in the backend to make sure it's not confused with the primary-key id, I thought it would be more clear to use the same terminology in the FE.

I don't have a strong opinion on this though. I think it will be clear enough regardless.

@acelaya acelaya force-pushed the dashboard-org-page branch from 62a225e to c8a012b Compare June 4, 2024 14:20
@acelaya acelaya merged commit f5fcb55 into main Jun 4, 2024
9 checks passed
@acelaya acelaya deleted the dashboard-org-page branch June 4, 2024 14:24
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