Skip to content
This repository has been archived by the owner on Apr 12, 2022. It is now read-only.

Add subnav items under each model #2953

Merged
merged 3 commits into from Feb 15, 2022
Merged

Add subnav items under each model #2953

merged 3 commits into from Feb 15, 2022

Conversation

rwfeather
Copy link
Contributor

@rwfeather rwfeather commented Feb 8, 2022

Change description

Adds new navigation items underneath the active model

Checklists

Development

  • Application changes have been tested appropriately

Impact

  • Code follows company security practices and guidelines
  • Security impact of change has been considered
  • Performance impact of change has been considered
  • Possible migration needs considered (model migrations, config migrations, etc.)

Please explain any security, performance, migration, or other impacts if relevant:

Code review

  • Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached where applicable.
  • Relevant tags have been added to the PR (bug, enhancement, internal, etc.)

@rwfeather
Copy link
Contributor Author

Screen.Recording.2022-02-09.at.9.26.36.AM.mov

@rwfeather rwfeather added the enhancement New feature or request label Feb 10, 2022
@rwfeather rwfeather marked this pull request as ready for review February 10, 2022 16:59
Copy link
Member

@evantahler evantahler left a comment

Choose a reason for hiding this comment

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

Looks great!

Comment on lines +128 to +135
{
type: "link",
title: "Records",
icon: "file-alt",
href: `/model/${model.id}/records`,
mainPathSectionIdx: 3,
small: true,
},
Copy link
Member

Choose a reason for hiding this comment

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

👍 I like that this goes to the list page

{childNav.title === "Runs" ? (
<>
{" "}
<RunningRunsBadge />
Copy link
Member

Choose a reason for hiding this comment

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

Oh! I guess we had lost this badge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this logic is still in the parent component: https://github.com/grouparoo/grouparoo/pull/2953/files#diff-f356a14f3edf616238baef014100f4a869eaa21d8bbf00df34305b2b420d3066L194

It got added here due to copy-paste, but after taking a second look it seems unnecessary to include it here. This code path handles the subNav items only, and since the "Runs" nav item is handled as a top level item behind the accordion, it doesn't really need to be here.

icon: string;
mainPathSectionIdx: number;
small: boolean;
navChildItems?: NavigationItem[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Maybe subNavItems ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, renamed.

@edmundito
Copy link
Contributor

I noticed in the video and confirmed in my local checkout as well that when navigating from the model's records to destinations it doesn't land in the right spot. It looks like the sample record is dynamically loading there instead of hydrating from getServerSideProps.

Also, I wonder if there is a way to enable smooth scrolling on the page after the first load, but nothing too complicated. maybe something simple based on this suggestion: https://stackoverflow.com/a/61784812/20494

@rwfeather
Copy link
Contributor Author

I noticed in the video and confirmed in my local checkout as well that when navigating from the model's records to destinations it doesn't land in the right spot. It looks like the sample record is dynamically loading there instead of hydrating from getServerSideProps.

Looks like I created this branch before #2941 landed in main. It should work after getting merged, but let me just do a quick rebase to find out

@rwfeather
Copy link
Contributor Author

Loading behavior looks better.

I'm currently working on adding the smooth scrolling.

@rwfeather rwfeather merged commit 0a0d0a1 into main Feb 15, 2022
@rwfeather rwfeather deleted the 180984300-model-links-nav branch February 15, 2022 00:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants