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

feat: changes policy type dropdown to in-page list #1356

Merged
merged 14 commits into from
Aug 23, 2023
Merged

feat: changes policy type dropdown to in-page list #1356

merged 14 commits into from
Aug 23, 2023

Conversation

kleinfreund
Copy link
Contributor

@kleinfreund kleinfreund commented Aug 21, 2023

Changes

Changes the policy type dropdown to an in-page list.

Splits out the presentational part of the policy list view into its own PolicyList component.

Resolves #1144.

Signed-off-by: Philipp Rudloff [email protected]

Screenshots

Before:

image

After:

image

@kleinfreund kleinfreund self-assigned this Aug 21, 2023
@netlify
Copy link

netlify bot commented Aug 21, 2023

Deploy Preview for kuma-gui ready!

Name Link
🔨 Latest commit 2ce8cf1
🔍 Latest deploy log https://app.netlify.com/sites/kuma-gui/deploys/64e4da34d3878b0008d9f8ef
😎 Deploy Preview https://deploy-preview-1356--kuma-gui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kleinfreund kleinfreund marked this pull request as ready for review August 21, 2023 08:28
@kleinfreund kleinfreund requested a review from a team as a code owner August 21, 2023 08:28
@kleinfreund kleinfreund requested review from johncowen and removed request for a team August 21, 2023 08:28
Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

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

There are a few odd things here.

  1. The actual implementation looks very different from the figma (missing top card with short desc and link to docs), Side selection shouldn't have borders, title should be just policies.
  2. We seem to be requesting the list of policies and the stats on each change of policy. This seems unnecessary
  3. For new policies we mentioned showing the kind of the top level targetRef to make it more useful (the current type column is useless as it's already selected on the left).
  4. Not found show errors (I don't know if it's a mock issue) but it should show the empty state.

@kleinfreund
Copy link
Contributor Author

kleinfreund commented Aug 21, 2023

This PR’s focus was to implement the change from the policy type dropdown to a sidebar.

  • The actual implementation looks very different from the figma (missing top card with short desc and link to docs),

Where do I get the descriptions from?

Side selection shouldn't have borders

We can’t currently avoid using cards around content like that. The designs assume that the background color of the page is a plain white, but that’s not always the case. On grey background, the content of the policy type list, for example, would look really bad (barely legible grey text on grey background). That’s why I asked for the designs to be done on a grey background so this would be taken into account automatically.

title should be just policies.

I’ve changed that.

  • We seem to be requesting the list of policies and the stats on each change of policy. This seems unnecessary

I can try and think of a way to avoid this.

Both currently and in this PR, selecting a policy type is a navigation to the policy list view with the appropriate parameter representing the policy type. That will always trigger the DataSources again.

  • For new policies we mentioned showing the kind of the top level targetRef to make it more useful (the current type column is useless as it's already selected on the left).

I have a branch prepared that implements this which is waiting for kumahq/kuma#7485 to be implemented. We need this so we know when to show the column.

  • Not found show errors (I don't know if it's a mock issue) but it should show the empty state.

Yeah, that’s a mock issue. We haven’t mocked out the data for all policy types (especially newer types) and so landing on such a view shows (correctly) the error state because with the mock data, the request does actually result in a 404. With the real API, this wouldn’t happen unless the policy type actually doesn’t exist.

Philipp Rudloff added 2 commits August 22, 2023 10:48
Changes the policy type dropdown to an in-page list.

Splits out the presentational part of the policy list view into its own PolicyList component.

Signed-off-by: Philipp Rudloff <[email protected]>
@lahabana
Copy link
Contributor

The actual implementation looks very different from the figma (missing top card with short desc and link to docs),

Where do I get the descriptions from?

For the description let's start with a generic text:

Use policies to apply filters to incoming or outgoing traffic of data plane proxies. To generate the Envoy configuration of a proxy the control-plane uses its data-plane configuration with the policies matching it.

Make sure there's a i18n entry so we can easily change it in the future.

Opening a PR for kumahq/kuma#7485 in a sec

Philipp Rudloff added 4 commits August 22, 2023 13:40
Signed-off-by: Philipp Rudloff <[email protected]>
Adds a new "Target ref" column to the policy list view.

Removes "Type" column.

Signed-off-by: Philipp Rudloff <[email protected]>
Signed-off-by: Philipp Rudloff <[email protected]>
@kleinfreund
Copy link
Contributor Author

  • Added the "About" section for policy types
  • Added inbound and outbound badges to the "About" section (used badges for now because we already have the beta badge now)
  • Pulled in the work for adding the target ref column because it will be available with feat(api-server): add isTargetRefBased in /policies kuma#7561
  • Removed the type column
  • Updated the /policies mock

Signed-off-by: Philipp Rudloff <[email protected]>
@kleinfreund
Copy link
Contributor Author

If the top-level target ref has a name and its kind is MeshService, does that represent a service we can link to (i.e. to the service detail view)?

Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

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

LGTM other than the few comments pointed out

Signed-off-by: Philipp Rudloff <[email protected]>
@kleinfreund kleinfreund merged commit f17cf0b into kumahq:master Aug 23, 2023
12 checks passed
@kleinfreund kleinfreund deleted the feat/change-policy-type-dropdown-to-list branch August 23, 2023 08:04
@kleinfreund kleinfreund removed their assignment Aug 23, 2023
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.

rework policy picker to not be a dropdown
3 participants