Skip to content

Add some basic UI improvements for api-gateway services#16476

Closed
andrewstucki wants to merge 4 commits intomainfrom
api-gateway-ui
Closed

Add some basic UI improvements for api-gateway services#16476
andrewstucki wants to merge 4 commits intomainfrom
api-gateway-ui

Conversation

@andrewstucki
Copy link
Copy Markdown
Contributor

@andrewstucki andrewstucki commented Mar 1, 2023

Description

There are a number of things we don't support yet, such as topologies, displaying upstreams, etc. for API gateways (the new VM-native ones), but I think these are the quickest things that we can add to clean up how our new gateways display in the UI. The fonts are slightly off due to running in proxying mode I guess?

Service Listing Page

Before:

Screen Shot 2023-02-28 at 9 57 39 PM

After:

Screen Shot 2023-02-28 at 9 56 42 PM

Search Bar

Before:

Screen Shot 2023-02-28 at 9 49 50 PM

After:

Screen Shot 2023-02-28 at 9 56 34 PM

Hovered Kind Label

Before:

Screen Shot 2023-02-28 at 9 50 11 PM

After:

Screen Shot 2023-02-28 at 9 56 04 PM

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

@andrewstucki andrewstucki added theme/api-gw Track API gateway work backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. labels Mar 1, 2023
@andrewstucki andrewstucki requested review from a team March 1, 2023 03:18
@github-actions github-actions bot added the theme/ui Anything related to the UI label Mar 1, 2023
Copy link
Copy Markdown
Member

@nathancoleman nathancoleman left a comment

Choose a reason for hiding this comment

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

Nice! Some really high-value improvements without any complexity 👍

Would like to see the currently-broken docs links fixed, but that's not an issue introduced by this PR

Comment on lines +35 to +37
(array 'ingress-gateway' '/consul/developer-mesh/ingress-gateways')
(array 'terminating-gateway' '/consul/developer-mesh/understand-terminating-gateways')
(array 'mesh-gateway' '/consul/developer-mesh/connect-gateways')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We only have the Learn guide covering API Gateway for Kubernetes, so I suppose it makes sense to leave API gateway out of this list for now

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was thinking we could add it eventually. As is it'll still show a learn guide link, but it'll just link to the root of the learn guides, which I think is fine, didn't want to change this component around too much.

Copy link
Copy Markdown
Contributor

@t-eckert t-eckert left a comment

Choose a reason for hiding this comment

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

Awesome followthrough on this, making sure the whole thing is buttoned down is great

Copy link
Copy Markdown
Contributor

@WenInCode WenInCode left a comment

Choose a reason for hiding this comment

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

For some reason the frontend tests on this are only running a noop job. Looking into it.

Changes look great though.

Name: computed('item.Kind', function () {
const name = normalizedGatewayLabels[this.item.Kind];
return name ? name : titleize(humanize(this.item.Kind));
}),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This works great as is, but if you'd like to update this component to use the newer ember (Octane) conventions for component you can update this to be:

import Component from '@glimmer/component';
import { titleize } from 'ember-cli-string-helpers/helpers/titleize';
import { humanize } from 'ember-cli-string-helpers/helpers/humanize';

const normalizedGatewayLabels = {
  'api-gateway': 'API Gateway',
  'mesh-gateway': 'Mesh Gateway',
  'ingress-gateway': 'Ingress Gateway',
  'terminating-gateway': 'Terminating Gateway',
};

export default class CounterComponent extends Component {
  get name() {
    const name = normalizedGatewayLabels[this.args.item.Kind];
    return name ? name : titleize(humanize(this.args.item.Kind));
  }
}

However, doing this will also require updating the associated template's usages of item and withInfo to be @item and @withInfo to denote the usage of passed in arguments.

Definitely not required, just providing it as an option.

https://guides.emberjs.com/release/components/component-arguments-and-html-attributes/#toc_arguments
and
https://ember-learn.github.io/ember-octane-vs-classic-cheat-sheet/#component-properties__tracked-vs-cp

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ooh, neat to know, that'd at least let us start removing some of the deprecation warnings that get thrown.

I'm thinking the bigger change can probably wait since we'll likely have some more changes going into this component in the next release i.e. at minimum, a learn tutorial -- and since we're trying to merge for a patch release, I don't want to make too many changes (especially not having touched Ember code for ~8 years).

Then the url should be /dc-1/services
And the title should be "Services - Consul"
Then I see 2 service models
Then I see 3 service models
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm seeing this test fail locally still showing 2 service models. I can try and look into it to help out later if I have some time after some meetings

@WenInCode
Copy link
Copy Markdown
Contributor

@andrewstucki any chance you could open this under a new branch name following the ui/branch-name branch name structure.

It looks like our CI frontend tests only run if the branch has that naming structure.

@valeriia-ruban
Copy link
Copy Markdown
Contributor

valeriia-ruban commented Mar 2, 2023

@andrewstucki any chance you could open this under a new branch name following the ui/branch-name branch name structure.

It looks like our CI frontend tests only run if the branch has that naming structure.

good catch Tyler 🏅

@andrewstucki
Copy link
Copy Markdown
Contributor Author

Closing in favor of #16508

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. theme/api-gw Track API gateway work theme/ui Anything related to the UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants