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

Sort NavigationTree alphabetically #112

Merged
merged 7 commits into from
Jun 21, 2023
Merged

Sort NavigationTree alphabetically #112

merged 7 commits into from
Jun 21, 2023

Conversation

prestonbourne
Copy link
Contributor

@prestonbourne prestonbourne commented Jun 20, 2023

Note that tests will continue to fail inaccurately until GH-110 is merged
Follow up for this PR
Solves #111
These changes do the following:

  1. Add a compare function to sort the NavigationTree using the built in sort method
  2. Creates a new ComponentEntity in examples/basic to better demonstrate the alphabetical sorting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a simple accordion to show the sorting

Comment on lines +8 to +15
export function Accordion({ children }: AccordionProps) {
return (
<details>
<summary>Hello 👋</summary>
Nice meeting you! How's everything going?
</details>
)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the built in HTML elements for this, I imagine down the line we'll use the components package Dylan is working on

Copy link
Contributor Author

@prestonbourne prestonbourne left a comment

Choose a reason for hiding this comment

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

Small changes to sort tree before exporting

Comment on lines 93 to 112
function compareTitleSort<T extends CategoryNode | NavigationNode>(
a: T,
b: T
): -1 | 0 | 1 {
const aHasChildren = 'children' in a && a.children.length > 0
const bHasChildren = 'children' in b && b.children.length > 0
if (aHasChildren) {
a.children.sort(compareTitleSort)
}
if (bHasChildren) {
b.children.sort(compareTitleSort)
}
if (a.title > b.title) {
return 1
}
if (b.title > a.title) {
return -1
}
return 0
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the compare function recursive per our discussion. @BRKalow

This way the tree will be sorted no matter how deep the structure gets as long as we keep using the children key to handle nesting

@prestonbourne prestonbourne requested a review from BRKalow June 20, 2023 13:55
@prestonbourne prestonbourne marked this pull request as ready for review June 20, 2023 13:55
Comment on lines 99 to 104
if (aHasChildren) {
a.children.sort(compareTitleSort)
}
if (bHasChildren) {
b.children.sort(compareTitleSort)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking this might not be the proper place to call sort recursively, as each element might get passed to the compare function more than once, which would result in us sorting an element's children multiple times. We'll likely want to separate out traversing the tree from the actual sorting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting, something like:

  • Create a toBeSorted = Map()
  • Traverse the tree, while checking which nodes have children
  • If hasChildren == true. Store reference to the node in toBeSorted
  • Loop through the map and sort

prestonbourne and others added 4 commits June 21, 2023 09:07
1. Rewrote the test for `parseComponentPath`
2. Added test for `getNavigationTree`
3. renamed `get-nav-tree.ts` to `get-navigation-tree.ts` 

Co-authored-by: Preston Bourne <[email protected]>
@prestonbourne prestonbourne requested a review from BRKalow June 21, 2023 13:53
Copy link
Contributor

@BRKalow BRKalow left a comment

Choose a reason for hiding this comment

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

👏 👏

@prestonbourne prestonbourne merged commit 15e6f2c into main Jun 21, 2023
@prestonbourne prestonbourne deleted the pres.sort-nav-tree branch June 21, 2023 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