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

Move Breadcrumbs and reconfigure for V2Routes #1550

Merged
merged 5 commits into from Mar 1, 2022
Merged

Move Breadcrumbs and reconfigure for V2Routes #1550

merged 5 commits into from Mar 1, 2022

Conversation

joshri
Copy link
Contributor

@joshri joshri commented Feb 28, 2022

Closes: #1509

image

Adds Breadcrumbs component, and should also fix some errors caused by data tables mixing with the new sorting pr.

Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

One blocker around window.location. Other than that, LGTM.

ui/components/Breadcrumbs.tsx Outdated Show resolved Hide resolved
ui/components/Breadcrumbs.tsx Outdated Show resolved Hide resolved
ui/components/DataTable.tsx Outdated Show resolved Hide resolved
ui/components/ReconciledObjectsTable.tsx Outdated Show resolved Hide resolved
},
{
label: "Namespace",
value: "namespace",
sortType: SortType.string,
sortValue: ({ namespace }) => namespace,
Copy link
Contributor

Choose a reason for hiding this comment

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

No longer necessary, right?

746b1be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My branch was out of date :( will fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ALSO - noticed a problem with this - right now DataTable is using the field having a SortType as an indication of whether it's sortable or not - we need a different indication of which columns are sortable

Copy link
Contributor

Choose a reason for hiding this comment

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

@joshri Still not fixed?

ui/components/__tests__/Breadcrumbs.test.tsx Show resolved Hide resolved
ui/components/__tests__/Breadcrumbs.test.tsx Show resolved Hide resolved
@joshri joshri requested a review from jpellizzari March 1, 2022 17:06
Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

Looks good. No blockers for me.

@@ -47,22 +46,6 @@ function AutomationsTable({ className, automations }: Props) {
label: "Namespace",
value: "namespace",
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not have expected these fields to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me neither put it back - I might have changed it trying to get the table to not break

},
{
label: "Namespace",
value: "namespace",
sortType: SortType.string,
sortValue: ({ namespace }) => namespace,
Copy link
Contributor

Choose a reason for hiding this comment

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

@joshri Still not fixed?

@@ -41,6 +42,24 @@ export const getParentNavValue = (
}
};

export const getPageLabel = (currentPage: string): string => {
const parsed = qs.parse(useLocation().search);
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Not sure how this is working. I was under the impression that you cannot call hooks from plain old JS like this. I thought it had to be within another hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me too but it works so I'm not asking questions

@joshri joshri merged commit e70c994 into v2 Mar 1, 2022
@joshri joshri deleted the breadcrumbs branch March 1, 2022 21:02
jpellizzari pushed a commit that referenced this pull request Mar 3, 2022
* breadcrumbs

* leftover in app context

* conflicts cleanup

* pr feedback

* small cleanups
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.

None yet

2 participants