Skip to content

Update policy violation details based on source #1313

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

Merged
merged 13 commits into from
Aug 18, 2022

Conversation

TheGostKasper
Copy link
Contributor

Update Policy violation details based on navigation source

) => {
return rows.map(r => {
return !!r.children ? (
<CanaryRowHeader rowkey={r.rowkey} value={undefined} key={r.rowkey}>
Copy link
Contributor

Choose a reason for hiding this comment

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

don't you think that component name need to be changed as it's not related to canary's view only ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd love to, this change will touch many files that i'd love not to include in this 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.

Those header breadcrumbs look wild to me 🤔. Is that long string intentional?

@@ -20,11 +20,13 @@ export enum FieldsType {
interface Props {
violations: PolicyValidation[];
tableType?: FieldsType;
sourcePath?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this can be optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the application violation uses it to pass the sourceType kustomization or helmRelease and the value is being used only in applicationFields .

It might not be optional then i'll update HeaderPath to create the route link but again both links are different and i'll update links ...etc

What do you think? Do you see it worth the change ?

@TheGostKasper
Copy link
Contributor Author

Those header breadcrumbs look wild to me thinking. Is that long string intentional?

I'm afraid yes as it display the Message instead of Policy-name, and as u see it's nice to trim it as it's the last path .
I'd go for trim it and leave the title in the page 🤔
image

@ahussein3 ahussein3 self-requested a review August 18, 2022 11:04
@ahussein3 ahussein3 dismissed their stale review August 18, 2022 11:13

will be handled in a separate PR

Copy link
Collaborator

@foot foot left a comment

Choose a reason for hiding this comment

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

Yeah, look okay to me 👍

@@ -19,6 +19,10 @@ const Container = styled.div`
`;
const Span = styled.span`
color: ${({ theme }) => theme.colors.white};
text-overflow: ellipsis;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now we're ellipsising it would be neat to add a title attr so you can know what it is.

@@ -40,7 +48,7 @@ function CanaryRowHeader({
<div className={classes.rowHeaderWrapper} data-testid={rowkey}>
<div className={classes.cardTitle}>{rowkey}:</div>
<span className={classes.body1}>
{value || '--'} {children}
{!!children ? children : value || '--'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work?

Suggested change
{!!children ? children : value || '--'}
{children || value || '--'}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and look at that beauty

count: policyViolationsCount,
},
{ label: data?.violation?.name || '' },
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we pull this out into its own page at some point and share a common main component to keep things like this a bit simpler.

@TheGostKasper TheGostKasper merged commit ab15a36 into main Aug 18, 2022
@TheGostKasper TheGostKasper deleted the policy-violation-details branch August 18, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants