Top level api nav #516

jake-bassett · 2021-01-21T03:33:12Z

Description

Renames mode toggle in Table controls to a more generic view toggle.
Changes the table mode toggle widget to a more generic view toggle widget breaks dependencies
Updates some of the URL paths, eliminating the top level "services" lists and using "apis" instead ("endpoints" was already used and didn't want to overload)

codecov · 2021-01-21T03:36:36Z

Codecov Report

Merging #516 (b493c68) into main (16e2732) will decrease coverage by 0.03%.
The diff coverage is 65.67%.

@@            Coverage Diff             @@
##             main     #516      +/-   ##
==========================================
- Coverage   85.59%   85.55%   -0.04%     
==========================================
  Files         754      755       +1     
  Lines       15504    15510       +6     
  Branches     1839     1839              
==========================================
- Hits        13270    13269       -1     
- Misses       2203     2210       +7     
  Partials       31       31

Impacted Files	Coverage Δ
src/app/entity-metadata.ts	`0.00% <0.00%> (ø)`
src/app/shared/navigation/navigation.component.ts	`100.00% <ø> (ø)`
.../widgets/table/table-widget-select-filter.model.ts	`46.66% <16.66%> (-13.34%)`	⬇️
...rd/widgets/table/table-widget-view-toggle.model.ts	`24.24% <26.66%> (ø)`
...d/widgets/table/table-widget-renderer.component.ts	`55.08% <50.00%> (-3.74%)`	⬇️
...dashboard/widgets/table/table-widget-base.model.ts	`78.37% <75.00%> (-4.48%)`	⬇️
...nts/src/table/controls/table-controls.component.ts	`96.07% <100.00%> (ø)`
...dashboard/widgets/table/table-widget-view.model.ts	`100.00% <100.00%> (ø)`
...red/dashboard/widgets/table/table-widget.module.ts	`100.00% <100.00%> (ø)`
...ability/src/pages/apis/endpoints-list.component.ts	`100.00% <100.00%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16e2732...b493c68. Read the comment docs.

aaron-steinfeld · 2021-01-21T15:17:46Z

projects/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-view.model.ts

+  public view!: string;
+
+  @ModelProperty({
+    key: 'json',


can we call this template (for consistency with other similar properties)

aaron-steinfeld · 2021-01-21T15:19:24Z

src/app/entity-metadata.ts

      icon: ObservabilityIconType.Api,
      detailPath: (id: string, sourceRoute?: string) => [sourceRoute ?? '', 'endpoint', id],
-      sourceRoutes: ['services']
+      sourceRoutes: ['apis']


I saw your comment, but unless I'm missing another spot, the only place endpoints is used is nested like service/endpoints. Rather than being confusing, I'd actually expect them to be consistent since they're showing the same list of entities in different scopes.

That's fine. I can go either way. I'll change apis to endpoints.

aaron-steinfeld · 2021-01-21T15:20:46Z

...ts/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-renderer.component.ts

    super.ngOnInit();

-    this.onModeChange(this.model.mode);
+    // This.onModeChange(this.model.mode);


no longer needed? how does the initial view get set? if no longer needed, remove comment

For this iteration, I don't have the different views in the URL, so not deep linkable. We just show the first view option on page load.

We need to decide how/if we want the different view toggles to be deep linkable (I think we do) and we need to decide if we want them as a route or a url param. I'm leaning towards param, but am open.

aaron-steinfeld · 2021-01-21T15:21:37Z

...s/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-select-filter.model.ts

  displayName: 'Select Filter'
 })
-export class TableWidgetSelectFilterModel {
+export class TableWidgetSelectFilterModel extends GraphQlDataSourceModel<PrimitiveValue[]> {


this isn't a data source itself - it's calling its api to get data.

…evel-api-nav

aaron-steinfeld · 2021-01-21T20:27:59Z

projects/observability/src/public-api.ts

 export * from './pages/api-trace-detail/api-trace-detail.page.component';

-export * from './pages/apis/services/service-list.module';
+export * from './pages/apis/endpoints-list.module';


I'm shocked you didn't put this with the component and dashboard in the same dir 😉

Hah, fail. It was actually because it started out as the services file and the IDE updated it when I renamed. I just moved it to its proper home though. Thanks!

aaron-steinfeld · 2021-01-21T20:46:12Z

src/app/entity-metadata.ts

-      detailPath: (id: string) => ['apis', 'service', id],
-      listPath: ['apis'],
-      apisListPath: (id: string) => ['apis', 'service', id, 'endpoints'],
+      listPath: ['endpoints'],


What are the desired breadcrumbs? Urls? I think things are a little off here (in terms of the entity metadata, the urls and the breadcrumb resolvers).

I think I'd expect:
Endpoint List => '/endpoints'
Endpoint => '/endpoint/' (should it be a different url if reached from service endpoint list vs endpoint list? different breadcrumb? If yes, then also '/service/endpoint/' and matching source path)
Service => '/service/' ; no list path, updated detail + api list path
Service Endpoint List => '/service//endpoints

I think I agree. Here is what we will have after the incoming changes.

I think the one that is still a bit of a question is endpoint.

My feelings are (assuming we want to show the service breadcrumb):

When navigated from a service: /service/endpoint/<endpoint-id>. This would correspond to breadcrumbs of: <Service Name> > Endpoints > <Endpoint name>

When navigated from elsewhere, such as EP list: /endpoint/<endpoint-id> This would correspond to breadcrumbs of: Endpoints > <Endpoint name>

Are we getting rid of the service listing screen?

How are we navigating to a service, when there is no service listing page? Via the service column in API endpoints list?

Why did we get rid of service screen? This was major thing in a customer perspective and they are using service dashboards for insights.

Is there an issue or discussion around? Can we add RATIONALE about this change? I came to know about this because of latest UI release I cut yesterday and was surprised with the change.

I am happy with moving APIs in first class navigation but that shouldn't result in removal of service nav completely.

cc: @jake-bassett @aaron-steinfeld

This conversation has happened in a few spots now, but duplicating here:
So in the weeds of the discussions around the endpoint list, it was overlooked that it should be a sibling of the service list, which will now be flat. We'll get that cleaned up today.

I put up #521 to fix the most pressing issues. It has significant, but not complete overlap with this PR (Sorry!)

…evel-api-nav

github-actions · 2021-01-21T21:31:41Z

Unit Test Results

    4 files ±0 232 suites ±0 12m 39s ⏱️ -23s
818 tests ±0 818 ✔️ ±0 0 💤 ±0 0 ❌ ±0
822 runs ±0 822 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit b493c68. ± Comparison against base commit 16e2732.

jake-bassett · 2021-01-22T20:26:29Z

@aaron-steinfeld did part of this work in another PR (thank you!). Going to close out this one.

jake-bassett added 2 commits January 20, 2021 19:16

feat: top level apis navigation

5c7dc31

fix: linting and tests

b1e785b

jake-bassett requested a review from a team as a code owner January 21, 2021 03:33

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Jan 21, 2021

View reviewed changes

jake-bassett added 4 commits January 21, 2021 10:42

fix: changed pathing to endpoints and other pr comments

2b87f61

Merge branch 'main' of github.com:hypertrace/hypertrace-ui into top-l…

8a336b8

…evel-api-nav

chore: remove debug tracing

e0eaa93

fix: linting

b979afd

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Jan 21, 2021

View reviewed changes

jake-bassett added 2 commits January 21, 2021 13:23

fix: url path changes - removed services/ path

c22eb70

Merge branch 'main' of github.com:hypertrace/hypertrace-ui into top-l…

b493c68

…evel-api-nav

jake-bassett closed this Jan 22, 2021

jake-bassett deleted the top-level-api-nav branch January 26, 2021 23:27

Top level api nav #516

Top level api nav #516

Uh oh!

Conversation

jake-bassett commented Jan 21, 2021

Description

Uh oh!

codecov bot commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jake-bassett Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaron-steinfeld Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JBAhire Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 21, 2021

Unit Test Results

Uh oh!

jake-bassett commented Jan 22, 2021

Uh oh!

Uh oh!

codecov bot commented Jan 21, 2021 •

edited

Loading

jake-bassett Jan 21, 2021 •

edited

Loading

aaron-steinfeld Jan 21, 2021 •

edited

Loading

JBAhire Jan 22, 2021 •

edited

Loading