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.

Sign up for GitHub

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

Jump to bottom

feat: Allow markdown in workflow title and description. Fixes #10126 #10553

Merged
merged 1 commit into from
Dec 30, 2023
Merged

feat: Allow markdown in workflow title and description. Fixes #10126 #10553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added docs/assets/workflow-title-and-description.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
14 changes: 14 additions & 0 deletions docs/fields.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,8 @@ Workflow is the definition of a workflow resource

- [`timeouts-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/timeouts-workflow.yaml)

- [`title-and-descriptin-with-markdown.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/title-and-descriptin-with-markdown.yaml)

- [`volumes-emptydir.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/volumes-emptydir.yaml)

- [`volumes-existing.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/volumes-existing.yaml)
Expand Down Expand Up @@ -755,6 +757,8 @@ WorkflowSpec is the specification of a Workflow.

- [`timeouts-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/timeouts-workflow.yaml)

- [`title-and-descriptin-with-markdown.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/title-and-descriptin-with-markdown.yaml)

- [`volumes-emptydir.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/volumes-emptydir.yaml)

- [`volumes-existing.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/volumes-existing.yaml)
Expand Down Expand Up @@ -1191,6 +1195,8 @@ CronWorkflowSpec is the specification of a CronWorkflow

- [`timeouts-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/timeouts-workflow.yaml)

- [`title-and-descriptin-with-markdown.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/title-and-descriptin-with-markdown.yaml)

- [`volumes-emptydir.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/volumes-emptydir.yaml)

- [`volumes-existing.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/volumes-existing.yaml)
Expand Down Expand Up @@ -3707,6 +3713,8 @@ MetricLabel is a single label for a prometheus metric
- [`pod-metadata.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/pod-metadata.yaml)

- [`steps-inline-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/steps-inline-workflow.yaml)

- [`title-and-descriptin-with-markdown.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/title-and-descriptin-with-markdown.yaml)
</details>

### Fields
Expand Down Expand Up @@ -4895,6 +4903,8 @@ ObjectMeta is metadata that all persisted resources must have, which includes al

- [`timeouts-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/timeouts-workflow.yaml)

- [`title-and-descriptin-with-markdown.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/title-and-descriptin-with-markdown.yaml)

- [`volumes-emptydir.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/volumes-emptydir.yaml)

- [`volumes-existing.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/volumes-existing.yaml)
Expand Down Expand Up @@ -5503,6 +5513,8 @@ A single application container that you want to run within a pod.

- [`timeouts-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/timeouts-workflow.yaml)

- [`title-and-descriptin-with-markdown.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/title-and-descriptin-with-markdown.yaml)

- [`volumes-emptydir.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/volumes-emptydir.yaml)

- [`volumes-existing.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/volumes-existing.yaml)
Expand Down Expand Up @@ -6238,6 +6250,8 @@ PersistentVolumeClaimSpec describes the common attributes of storage devices and

- [`timeouts-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/timeouts-workflow.yaml)

- [`title-and-descriptin-with-markdown.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/title-and-descriptin-with-markdown.yaml)

- [`volumes-emptydir.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/volumes-emptydir.yaml)

- [`volumes-existing.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/volumes-existing.yaml)
Expand Down
25 changes: 25 additions & 0 deletions docs/title-and-description.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Title and Description

agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
If you add specific title and description annotations to your workflow they will show up on the workflow lists. It will also work with markdown.

```yaml
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
name: my-wf
annotations:
workflows.argoproj.io/title: '**Build and test**'
workflows.argoproj.io/description: '`SuperDuperProject` PR #6529: Implement frobbing (aff39ee)'
```

Examples by row:
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved

- markdown title, markdown description using [`remark-gfm`](https://github.com/remarkjs/remark-gfm) to auto turn URLs into links
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
- no title or description, defaults to `workflow.metadata.name`
- markdown title, no description
- no title, markdown description (title defaults to `workflow.metadata.name`)
- markdown title, markdown description (description includes URL that auto turns into anchor
- markdown title, markdown description, includes markdown URL
- markdown title, markdown description, includes markdown URL (pr link goes to `github.com`)

![Workflow Title And Description](assets/workflow-title-and-description.png)
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
19 changes: 19 additions & 0 deletions examples/title-and-descriptin-with-markdown.yaml
View file
Open in desktop
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: title-and-description-with-markdown-
labels:
workflows.argoproj.io/archive-strategy: "false"
annotations:
workflows.argoproj.io/title: "**Test Title**"
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
workflows.argoproj.io/description: |
`This is a simple hello world example.`
You can also run it in Python: https://couler-proj.github.io/couler/examples/#hello-world
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
spec:
entrypoint: whalesay
templates:
- name: whalesay
container:
image: docker/whalesay:latest
command: [cowsay]
args: ["hello world"]
1 change: 1 addition & 0 deletions mkdocs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ nav:
- artifact-visualization.md
- widgets.md
- intermediate-inputs.md
- title-and-description.md
- Debugging Tools:
- workflow-events.md
- debug-pause.md
Expand Down
2 changes: 2 additions & 0 deletions ui/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@
"react-chartjs-2": "^2.11.2",
"react-datepicker": "^4.25.0",
"react-dom": "^16.14.0",
"react-markdown": "^8.0.7",
"react-monaco-editor": "^0.50.1",
"react-router-dom": "^4.2.2",
"remark-gfm": "^3.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need GFM? The features it adds is fairly small, but it is an entire new dep. Most of those features would also not be used, e.g. tables and task lists don't really make sense in this context.

In general, a smaller dep that supports less features would make more sense, as full-featured Markdown in an annotation has some issues and being displayed inside of a list also is a specific context

Given that I spent a solid amount of time improving the UI load time with code-splitting for #12059, we really should be even more careful than we already should be when introducing new, large deps, especially in the UI

Copy link
Member

Choose a reason for hiding this comment

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

On Slack we discussed potentially putting the whole markdown feature behind a feature flag as otherwise the deps load unconditionally for all users, even those who don't use this feature

Copy link
Member

Choose a reason for hiding this comment

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

So we can do an initial code-split and only load the deps if the title and description annotations exist, but it may be difficult to discern if those have markdown specifically without a dep, which is where a feature flag might be good. I still would prefer not to gate it behind a feature flag, but I also don't want the deps loaded for users who don't use the feature.

Copy link
Member

Choose a reason for hiding this comment

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

Code-split this in #12580 based on if the annotations exist, but as written above, can't quite determine if those have markdown without a dep, so there is potentially further action to do here. Maybe there's a clever one-line regex to do it though and then we wouldn't need a dep to detect markdown

"superagent": "^8.1.2",
"swagger-ui-react": "^4.19.1"
},
Expand Down
20 changes: 20 additions & 0 deletions ui/src/app/workflows/components/workflows-row/workflows-row-name.tsx
View file
Open in desktop
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import * as kubernetes from 'argo-ui/src/models/kubernetes';
import * as React from 'react';
import ReactMarkdown from 'react-markdown';
import remarkGfm from 'remark-gfm';
import {ANNOTATION_DESCRIPTION, ANNOTATION_TITLE} from '../../../shared/annotations';

require('./workflows-row.scss');

export const WorkflowsRowName = ({metadata}: {metadata: kubernetes.ObjectMeta}) => {
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
const title = (metadata.annotations && metadata.annotations[ANNOTATION_TITLE]) || metadata.name;
const description = (metadata.annotations && metadata.annotations[ANNOTATION_DESCRIPTION] && `\n${metadata.annotations[ANNOTATION_DESCRIPTION]}`) || '';
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
const markdown = `${title}${description}`;
return (
<div className='wf-rows-name'>
<ReactMarkdown components={{p: React.Fragment}} remarkPlugins={[remarkGfm]}>
{markdown}
</ReactMarkdown>
</div>
);
};
22 changes: 22 additions & 0 deletions ui/src/app/workflows/components/workflows-row/workflows-row.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
@import 'node_modules/argo-ui/src/styles/config';

.wf-rows-name {
line-height: 1.5em;
display: inline-block;
position: relative;
pointer-events: none;
vertical-align: middle;
white-space: pre-line;
}
.overlay {
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
position: absolute;
top: 0;
left: 0;
right: 0;
bottom: 0;
}
jmeridth marked this conversation as resolved.
Show resolved Hide resolved
.wf-rows-name a {
pointer-events: all;
position: relative;
z-index: 1;
}
13 changes: 7 additions & 6 deletions ui/src/app/workflows/components/workflows-row/workflows-row.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@ import {useState} from 'react';
import {Link} from 'react-router-dom';
import * as models from '../../../../models';
import {isArchivedWorkflow, Workflow} from '../../../../models';
import {ANNOTATION_DESCRIPTION, ANNOTATION_TITLE} from '../../../shared/annotations';
import {uiUrl} from '../../../shared/base';
import {DurationPanel} from '../../../shared/components/duration-panel';
import {PhaseIcon} from '../../../shared/components/phase-icon';
import {Timestamp} from '../../../shared/components/timestamp';
import {wfDuration} from '../../../shared/duration';
import {WorkflowDrawer} from '../workflow-drawer/workflow-drawer';
import {WorkflowsRowName} from './workflows-row-name';

interface WorkflowsRowProps {
workflow: Workflow;
Expand All @@ -23,6 +23,7 @@ interface WorkflowsRowProps {
export function WorkflowsRow(props: WorkflowsRowProps) {
const [hideDrawer, setHideDrawer] = useState(true);
const wf = props.workflow;
const workflowLink = uiUrl(`workflows/${wf.metadata.namespace}/${wf.metadata.name}?uid=${wf.metadata.uid}`);

return (
<div className='workflows-list__row-container'>
Expand All @@ -41,16 +42,16 @@ export function WorkflowsRow(props: WorkflowsRowProps) {
/>
<PhaseIcon value={wf.status.phase} />
</div>
<div className='columns small-2'>
<a className='overlay' href={workflowLink}></a>
Copy link
Member

@agilgur5 agilgur5 Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is indeed quite hacky as you mentioned in the PR description. I also can't seem to expand the row right now, clicking the "Show" button takes me to the Workflow Details page.

instead of using an a tag, you can use a different element and capture the event and then use navigation.goto to change the route (which is what the Link component does under-the-hood). clicks elsewhere should capture the event first and prevent it from bubbling up to this level.

but tbh, I don't like that the whole thing is a link in the first place, that's very confusing UX-wise as there are different behaviors based on where/when you click. and there's a bunch of text that one may want to copy+paste or highlight, which is basically impossible right now.
so I actually think we should change up the behavior anyway, but I'm not yet quite sure where to put the link to the details page then. maybe we just make the name link to to the Workflow Details and remove the link from everywhere else.

Copy link
Member

@agilgur5 agilgur5 Jan 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So I removed the whole row being a link in #12580 and made the name only a link.
But then the nested linking issue re-appeared, so I used JS as I mentioned above to prevent bubble up, which is a lot less hacky and less problematic then the overlay CSS (although it still has some semantic issues, as I mention in #12580, but they're technically an edge of an edge case)

<WorkflowsRowName metadata={wf.metadata} />
</div>
<Link
to={{
pathname: uiUrl(`workflows/${wf.metadata.namespace}/${wf.metadata.name}`),
search: `?uid=${wf.metadata.uid}`
}}
className='small-11 row'>
<div className='columns small-2'>
{(wf.metadata.annotations && wf.metadata.annotations[ANNOTATION_TITLE]) || wf.metadata.name}
{wf.metadata.annotations && wf.metadata.annotations[ANNOTATION_DESCRIPTION] ? <p>{wf.metadata.annotations[ANNOTATION_DESCRIPTION]}</p> : null}
</div>
className='small-8 row'>
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
<div className='columns small-1'>{wf.metadata.namespace}</div>
<div className='columns small-1'>
<Timestamp date={wf.status.startedAt} />
Expand Down
Loading