Skip to content

Conversation

aali309
Copy link
Collaborator

@aali309 aali309 commented Aug 27, 2025

See: GITOPS-7298

This PR adds a custom details page that overrides the provided one by the console

Screenshot 2025-08-27 at 10 15 14 PM Screenshot 2025-08-27 at 10 19 32 PM Screenshot 2025-08-27 at 10 19 04 PM Screenshot 2025-08-27 at 10 18 43 PM

@@ -106,39 +102,10 @@ const ApplicationList: React.FC<ApplicationProps> = ({
return sortData(applications, sortBy, direction);
}, [applications, sortBy, direction]);
// TODO: use alternate filter since it is deprecated. See DataTableView potentially
const [data, filteredData, onFilterChange] = useListPageFilter(sortedApplications, filters);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aali309 , what's the reason for removing this? We no longer get the message saying that there are no applications in the project(s)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To just display the apps generated by the appset, its fixed now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, just have to consider both cases. Applications on their own, and applications related to the appset that is passed in.


// Determine the correct icon text and styling based on the model
const isApplicationSet = model.kind === 'ApplicationSet';
const iconText = isApplicationSet ? 'AS' : 'A';
Copy link
Collaborator

@keithchong keithchong Aug 28, 2025

Choose a reason for hiding this comment

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

Maybe provide these const's as properties? We will have another details page for App Projects.

Also we should rename this and move it to where DetailsPageTitle currently is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

{/* Tabs Section */}
<div className="co-m-pane__body">
<Tabs activeKey={activeTabKey} onSelect={handleTabClick} className="pf-v6-c-tabs">
<Tab eventKey={0} title={<TabTitleText>Details</TabTitleText>} className="pf-v6-c-tab-content">
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need this border here. The other console pages do not have this.

Screenshot 2025-08-28 at 1 39 52 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

</div>
</Tab>

<Tab eventKey={1} title={<TabTitleText>YAML</TabTitleText>} className="pf-v6-c-tab-content">
Copy link
Collaborator

@keithchong keithchong Aug 28, 2025

Choose a reason for hiding this comment

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

This YAML view should look similar to other YAML views, and it isn't properly themed (The text is not colorized):

CurrentYaml

Compare this to the YAML tab for a Deployment:

YamlForDeployment

<CardTitle>ApplicationSet details</CardTitle>
</CardHeader>
<CardBody>
<DescriptionList data-test-id="resource-summary">
Copy link
Collaborator

Choose a reason for hiding this comment

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

We will probably pull these common attributes (Name, Namespace, Labels, Annotations, etc.) to a common component.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

</CardHeader>
<CardBody>
<DescriptionList data-test-id="resource-summary">
<div className="pf-v6-c-description-list__group">
Copy link
Collaborator

@keithchong keithchong Aug 28, 2025

Choose a reason for hiding this comment

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

These can be replaced by pattern fly's <DescriptionListGroup >, <DescriptionListDescription> and make use of <DescriptionListTermHelpText> and <DescriptionListTermHelpTextButton>

<CardBody>
<DescriptionList data-test-id="resource-summary">
<div className="pf-v6-c-description-list__group">
<dt className="pf-v6-c-description-list__term" data-test-selector="details-item-label_Name">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll try to create a draft PR of my Application Details Page Details Tab. I don't have that many defined styles (which should be pulled out to the scss file).

right: 0,
fontSize: 13,
}}
aria-label="Edit labels"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should enable or disable the button if we know whether this resource can be updated.

</dd>
</div>

{/* Generators Section */}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on my new changes all borders are fixed

</div>
</Tab>

<Tab eventKey={4} title={<TabTitleText>Events</TabTitleText>} className="pf-v6-c-tab-content">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar rounded border here that is unneeded.

</div>
</Tab>

<Tab eventKey={2} title={<TabTitleText>Generators</TabTitleText>} className="pf-v6-c-tab-content">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar 'extra' rounded border here, like in the Details tab. See my previous comment.

</CardHeader>
<CardBody>
<div style={{ display: 'flex', flexDirection: 'column', gap: '16px' }}>
{appSet.spec?.generators?.map((generator: any, index: number) => {
Copy link
Collaborator

@keithchong keithchong Aug 28, 2025

Choose a reason for hiding this comment

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

For an appset with a matrix generator, I don't see the individual generators listed under this tab. Similarly, check your list generator, I don't see the related details for it.

@aali309 aali309 requested a review from keithchong August 29, 2025 10:32
@aali309 aali309 marked this pull request as ready for review August 29, 2025 10:32
@openshift-ci openshift-ci bot requested a review from wtam2018 August 29, 2025 10:33
{generator.directories && (
<DescriptionListGroup>
<DescriptionListTerm>Directories</DescriptionListTerm>
<DescriptionListDescription>{generator.directories.length} directory(ies)</DescriptionListDescription>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should explicitly say: 1 directory
and for more than 1, say: 2 directories
It will be more polished that way

<DescriptionListDescription>
<pre style={{
fontSize: '12px',
backgroundColor: '#f6f6f6',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't look right in dark theme or light theme. To test it out, you can change the theme from the top right drop down of the Console UI (usually the login name, or Auth disabled), and select User Preferences

</Popover>
</DescriptionListTermHelpText>
<DescriptionListDescription>
<a href="https://github.com/aal/309/argocd-test-nested.git" target="_blank" rel="noopener noreferrer">
Copy link
Collaborator

Choose a reason for hiding this comment

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

These URLs are hardcoded here :-)

<PaneHeading>
<Title headingLevel="h1">
<span
className="co-m-resource-icon co-m-resource-icon--lg"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the application-details-title.scss (in src/gitops/components/application) needs to be moved over to this folder and renamed resource-details-title.scss. The original class name I had was argocd-application-icon, so call it argocd-resource-icon and add it to this here. The text (A or AS) doesn't have the proper background colour.

I originally only had:

.argocd-application-icon {
    background-color: #E9654B;
}

Do we need all those styles that are currently in application-detials-title.scss?

resourcePrefix="Argo CD"
showDevPreviewBadge={true}
/>
<div className="application-set-details-page__body">
Copy link
Collaborator

@keithchong keithchong Aug 29, 2025

Choose a reason for hiding this comment

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

I believe in this case, we shouldn't use PF. we should use <HorizontalNav> from @openshift-console/dynamic-plugin-sdk

If you currently click on the various tabs, you should be able to press the browser's Back and Forward buttons to navigate to the tabs again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's also some extra padding on the left side within the contents of each tab, which might be fixed by the change to HorizontalNav

Spacing

)}
</Title>
<div className="co-actions" style={{ display: 'flex', alignItems: 'center', gap: '16px' }}>
<FavoriteButton defaultName={name ?? obj?.metadata?.name} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

This Favorites button doesn't add the page to Favorites section.

I think for now, we should not include this. We got it "for free" when using ListPageHeader for the two List pages, and it works there.

@@ -0,0 +1,25 @@
import * as React from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move this to a shared component or a utils component? The Applications YAML tab will use this too

Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
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.

2 participants