feat: Allow markdown title and description in CronWorkflows, WorkflowTemplates, & ClusterWorkflowTemplates. Fixes #12644 #12697

panicboat · 2024-02-25T06:40:34Z

Fixes #12644

Motivation

Use markdown for better visibility

Modifications

Allow markdown in WorkflowTemplate.
Allow markdown in CronWorkflow
Allow markdown in ClusterWorkflowTemplate

Verification

no annotations
only title with markdown
only description with markdown
title and description with markdown

agilgur5

I mentioned in the issue that you should use the code from #12580 which contains various fixes, optimizations, and more.

This should also add new sections for all 3 of these to the docs from #12582. Could just add screenshots to those sections, don't need to give examples again since they're effectively the same amongst these. Using the same example text as the existing docs would be desired for consistency as well (your screenshots here have different text, so would not suffice)

ui/src/app/cluster-workflow-templates/cluster-workflow-template-list.scss

agilgur5

There's a few issues with all the copy+pasting here. In the components below, copy+pasting is not the right approach -- importing would be better. In the CSS, the copy+pasted code seems largely irrelevant.
Please be thoughtful about what and how you copy+paste.

I've also requested some changes to the docs.

See in-line comments below

ui/src/app/cluster-workflow-templates/react-markdown-gfm.tsx

ui/src/app/cluster-workflow-templates/cluster-workflow-template-row.tsx

agilgur5 · 2024-03-03T22:00:38Z

ui/src/app/cluster-workflow-templates/cluster-workflow-template-list.tsx

-                    {templates.map(t => (
-                        <Link className='row argo-table-list__row' key={t.metadata.uid} to={uiUrl(`cluster-workflow-templates/${t.metadata.name}`)}>
-                            <div className='columns small-1'>
-                                <i className='fa fa-clone' />
-                            </div>
-                            <div className='columns small-5'>{t.metadata.name}</div>
-                            <div className='columns small-3'>
-                                <Timestamp date={t.metadata.creationTimestamp} />
-                            </div>
-                        </Link>
-                    ))}
+                    {templates.map(t => {
+                        return <ClusterWorkflowTemplateRow workflow={t} key={`${t.metadata.namespace}/${t.metadata.name}`} />;
+                    })}


Conversely, I don't think this row needs to be split into a separate component at this time. It can be left as-is within the list component. The row is quite short here (note that the Workflows Row has much more details than this row)

It would be easier to renovate the system in the future if it were structured in the same way.
Personally, I would like to keep it the same. Is that a problem?

Ah I see what you were going for, because workflows-row is split it out, make the others consistent.

Two things:

The Workflows List is the exception to things because it's so big and detailed; I'm not sure that other components should follow its example

If we do want to refactor this as such, it would be better done as a separate refactor PR. That keeps things "atomic" and single purpose -- in particular, it keeps the diff here to only what is necessary for this feature, which makes it easier to review now and easier to understand in the future if someone looks at this PR (if this needs a revert for some reason, for historical purposes, or if someone want to copy this PR to another feature, etc). Similarly a refactor is easier to read / review / revert etc as its own independent PR -- the changes should effectively be a no-op as they do not change behavior.

Also I see that the variables here are a bit harder to use without a separated component. That is a bit annoying.

There's a few techniques to workaround that. Two main ones I am thinking of are to call a function to do some of the logic and to split out the title/desc as its own component, but in the same file for now. The latter would be more consistent with the Workflows Row and would be easier to follow-up with a refactor if desired

Okay, I made the title and description separate components.

ui/src/app/cluster-workflow-templates/cluster-workflow-template-list.scss

ui/src/app/workflows/components/workflows-row/workflows-row.tsx

docs/title-and-description.md

docs/assets/cluster-workflow-template-title-and-description-markdown-complex.png

docs/assets/cluster-workflow-template-title-and-description-markdown.png

agilgur5

This is looking much better! 🙂

I left several small documentation comments in-line below, as well as a few code comments. I haven't reviewed the row components as I think it'd be easier to do so if those are not refactored in this PR.

Also DCO is missing on one of your commits here

docs/title-and-description.md

agilgur5 · 2024-03-05T03:34:52Z

ui/src/app/cluster-workflow-templates/cluster-workflow-template-list.tsx

-                    {templates.map(t => (
-                        <Link className='row argo-table-list__row' key={t.metadata.uid} to={uiUrl(`cluster-workflow-templates/${t.metadata.name}`)}>
-                            <div className='columns small-1'>
-                                <i className='fa fa-clone' />
-                            </div>
-                            <div className='columns small-5'>{t.metadata.name}</div>
-                            <div className='columns small-3'>
-                                <Timestamp date={t.metadata.creationTimestamp} />
-                            </div>
-                        </Link>
-                    ))}
+                    {templates.map(t => {
+                        return <ClusterWorkflowTemplateRow workflow={t} key={`${t.metadata.namespace}/${t.metadata.name}`} />;
+                    })}


Ah I see what you were going for, because workflows-row is split it out, make the others consistent.

Two things:

The Workflows List is the exception to things because it's so big and detailed; I'm not sure that other components should follow its example

If we do want to refactor this as such, it would be better done as a separate refactor PR. That keeps things "atomic" and single purpose -- in particular, it keeps the diff here to only what is necessary for this feature, which makes it easier to review now and easier to understand in the future if someone looks at this PR (if this needs a revert for some reason, for historical purposes, or if someone want to copy this PR to another feature, etc). Similarly a refactor is easier to read / review / revert etc as its own independent PR -- the changes should effectively be a no-op as they do not change behavior.

ui/src/app/shared/components/suspense-react-markdown-gfm.tsx

agilgur5 · 2024-03-05T03:42:12Z

ui/src/app/shared/components/react-markdown-gfm.tsx

could you make an additional small modification to this file? now that it's shared, I think we could be more explicit that this component is not meant to be used in isolation and that instead the lazy-loaded suspense variant should be used.

Could you add an _ underscore prefix to the name of the file, i.e. _react-markdown-gfm.tsx to give some form of indication that it's "private"? (the underscore prefix does not actually do anything in JS/TS, but people with experience in other languages might recognize it).

Also could you add at the top of the file a comment saying to use the lazy loaded suspense variant instead with its filename listed? GitHub won't let me comment in-line at the top because the first line is unchanged

Thanks for your support.
I learned a lot.
I've made some corrections, can you check them?

terrytangyuan

@jmeridth Would you like to help review this once it's ready?

Signed-off-by: panicboat <panicboat@gmail.com> fix: review points on CronWorkflow Signed-off-by: panicboat <panicboat@gmail.com> fix: review points Signed-off-by: panicboat <panicboat@gmail.com> fix: review points Signed-off-by: panicboat <panicboat@gmail.com> fix: review point Signed-off-by: panicboat <panicboat@gmail.com> fix: review point Signed-off-by: panicboat <panicboat@gmail.com> Update ui/src/app/cluster-workflow-templates/cluster-workflow-template-list.scss Signed-off-by: panicboat <panicboat@gmail.com> fix: review points Signed-off-by: panicboat <panicboat@gmail.com> fix: review points Signed-off-by: panicboat <panicboat@gmail.com> fix: spelling error fix: rename wf name Signed-off-by: panicboat <panicboat@gmail.com> Apply suggestions from code review Signed-off-by: panicboat <panicboat@gmail.com> feat: Allow markdown title and description. Signed-off-by: panicboat <panicboat@gmail.com> fix: review points Signed-off-by: panicboat <panicboat@gmail.com> Co-Authored-By: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>

panicboat · 2024-11-15T04:38:54Z

Hi.
I have confirmed that the equivalent of this PR has been released in v3.6.
Can I Close this PR in the future as Merge will no longer be needed for it?

agilgur5 · 2024-11-15T05:39:52Z

It wasn't, only your non-markdown PR #12674 made it in.

For reference, I did specifically ask for this to be included in 3.6 on more than one occasion and specifically put it in the 3.6.0 milestone for that reason

panicboat · 2024-11-15T07:41:45Z

@agilgur5 Thanks, I understand.
I should have fixed the conflicts.
I will actively fix the conflicts from now on.
I will send you another PR for markdown support, so please let me close this PR.
Or does it matter if I modify this PR?

agilgur5 · 2024-11-15T16:47:03Z

I should have fixed the conflicts.
I will actively fix the conflicts from now on.

That would certainly help, but I don't think you did anything wrong. I just didn't have time to review this as I had plenty of other things on my plate, especially as this was a 3.6 feature and 3.5 had many high priority bugs.
Others did not step in to help review either. Any Approver+ can also write to your branch by default and any contributor can fix conflicts in a new PR that credits your original authorship.

Or does it matter if I modify this PR?

It is usually better to re-use an existing PR rather than making a new one to keep the history and have it be easier to search than duplication.

That being said, I no longer have the ability to approve PRs as I resigned recently, in one part due to arbitrary decision changes around 3.6 made without even a discussion and without commenting on any existing issues/PRs/meetings and that being part of a broader pattern that I find antithetical to the open and collaborative nature of open source.

panicboat changed the title ~~feat: Allow markdown title and description.~~ feat: Allow markdown title and description. Fixes #12644 Feb 25, 2024

panicboat marked this pull request as ready for review February 25, 2024 07:27

agilgur5 changed the title ~~feat: Allow markdown title and description. Fixes #12644~~ feat: Allow markdown title and description in CronWorkflows & WorkflowTemplates. Fixes #12644 Feb 25, 2024

agilgur5 self-assigned this Feb 25, 2024

agilgur5 added the area/ui label Feb 25, 2024

agilgur5 suggested changes Feb 25, 2024

View reviewed changes

agilgur5 added area/cron-workflows area/workflow-templates labels Feb 25, 2024

panicboat force-pushed the panicboat/allow-for-markdown-in-workflow-title-and-description branch from b1a0428 to b0b528f Compare February 26, 2024 09:56

panicboat marked this pull request as draft February 26, 2024 09:56

panicboat changed the title ~~feat: Allow markdown title and description in CronWorkflows & WorkflowTemplates. Fixes #12644~~ feat: Allow markdown title and description in CronWorkflows & WorkflowTemplates, ClusterWorkflowTemplates. Fixes #12644 Mar 3, 2024

panicboat commented Mar 3, 2024

View reviewed changes

ui/src/app/cluster-workflow-templates/cluster-workflow-template-list.scss Outdated Show resolved Hide resolved

panicboat force-pushed the panicboat/allow-for-markdown-in-workflow-title-and-description branch from 657d309 to 1824504 Compare March 3, 2024 11:23

panicboat marked this pull request as ready for review March 3, 2024 11:30

panicboat requested a review from agilgur5 March 3, 2024 11:30

agilgur5 changed the title ~~feat: Allow markdown title and description in CronWorkflows & WorkflowTemplates, ClusterWorkflowTemplates. Fixes #12644~~ feat: Allow markdown title and description in CronWorkflows, WorkflowTemplates, & ClusterWorkflowTemplates. Fixes #12644 Mar 3, 2024

agilgur5 suggested changes Mar 3, 2024

View reviewed changes

panicboat requested a review from agilgur5 March 4, 2024 17:04

panicboat marked this pull request as draft March 5, 2024 03:12

agilgur5 reviewed Mar 5, 2024

View reviewed changes

terrytangyuan reviewed Mar 5, 2024

View reviewed changes

panicboat force-pushed the panicboat/allow-for-markdown-in-workflow-title-and-description branch from 6d5030c to 793b48a Compare March 5, 2024 11:03

panicboat force-pushed the panicboat/allow-for-markdown-in-workflow-title-and-description branch from 793b48a to 028eb74 Compare March 5, 2024 13:19

panicboat requested a review from agilgur5 March 5, 2024 13:25

panicboat marked this pull request as ready for review March 5, 2024 13:25

jmeridth approved these changes Mar 5, 2024

View reviewed changes

agilgur5 added this to the v3.6.0 milestone Apr 12, 2024

agilgur5 removed their assignment Nov 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow markdown title and description in CronWorkflows, WorkflowTemplates, & ClusterWorkflowTemplates. Fixes #12644 #12697

feat: Allow markdown title and description in CronWorkflows, WorkflowTemplates, & ClusterWorkflowTemplates. Fixes #12644 #12697

panicboat commented Feb 25, 2024 •

edited

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 Mar 3, 2024

panicboat Mar 4, 2024

agilgur5 Mar 5, 2024

agilgur5 Mar 5, 2024 •

edited

Loading

panicboat Mar 5, 2024

agilgur5 left a comment •

edited

Loading

agilgur5 Mar 5, 2024

agilgur5 Mar 5, 2024 •

edited

Loading

panicboat Mar 5, 2024

terrytangyuan left a comment •

edited

Loading

panicboat commented Nov 15, 2024

agilgur5 commented Nov 15, 2024

panicboat commented Nov 15, 2024 •

edited

Loading

agilgur5 commented Nov 15, 2024 •

edited

Loading

feat: Allow markdown title and description in CronWorkflows, WorkflowTemplates, & ClusterWorkflowTemplates. Fixes #12644 #12697

Are you sure you want to change the base?

feat: Allow markdown title and description in CronWorkflows, WorkflowTemplates, & ClusterWorkflowTemplates. Fixes #12644 #12697

Conversation

panicboat commented Feb 25, 2024 • edited Loading

Motivation

Modifications

Verification

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

agilgur5 Mar 3, 2024

Choose a reason for hiding this comment

panicboat Mar 4, 2024

Choose a reason for hiding this comment

agilgur5 Mar 5, 2024

Choose a reason for hiding this comment

agilgur5 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

panicboat Mar 5, 2024

Choose a reason for hiding this comment

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

agilgur5 Mar 5, 2024

Choose a reason for hiding this comment

agilgur5 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

panicboat Mar 5, 2024

Choose a reason for hiding this comment

terrytangyuan left a comment • edited Loading

Choose a reason for hiding this comment

panicboat commented Nov 15, 2024

agilgur5 commented Nov 15, 2024

panicboat commented Nov 15, 2024 • edited Loading

agilgur5 commented Nov 15, 2024 • edited Loading

panicboat commented Feb 25, 2024 •

edited

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 Mar 5, 2024 •

edited

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 Mar 5, 2024 •

edited

Loading

terrytangyuan left a comment •

edited

Loading

panicboat commented Nov 15, 2024 •

edited

Loading

agilgur5 commented Nov 15, 2024 •

edited

Loading