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

fix(ui): improve markdown styles for workflow-row, workflow-templates, cluster-workflow-templates, and cron-workflows #13930

Merged
merged 26 commits into from
Dec 10, 2024

Conversation

stevenbjohnson
Copy link
Contributor

@stevenbjohnson stevenbjohnson commented Nov 23, 2024

Motivation

These changes are being proposed to improve the workflows-row component. With the addition of the .wf-rows-name class for displaying the title and description annotations, previous styles that handled overflow and text-overflow are being overridden. This results in lengthy titles and descriptions creating wrapped text that renders inconsistently sized workflow-row components and makes details difficult to read.

Similarly, there are some markdown text formats that cause the UI to render the workflow-row component inconsistently and make the title and description equally as difficult to read.

Modifications

  • Updated workflows-row.scss styles for the .wf-rows-name class
    • increased line-height by 1em
    • removed inline-block display
    • removed middle vertical alignment (useless without inline-block)
    • added styles to properly handle overflow and text-overflow without wrapping (applies to all child elements, as well)

Before Change:
image

After Change:
image

  • added conditional to the start/finish columns for adding the workflows-list__timestamp class, ensuring times remain aligned in workflows-row when not ISO format (the above changes will create misalignment when a title and description are present)

Before Change:
image

After Change:
image
image
image

  • added EscapeInvalidMarkdown() to prevent markdown text formats that cause large variances in workflow-row sizing, as well as complicate readability (markdown that starts with a header, blockquotes, or list items will have characters removed):

Supported formatting:

  1. Bold ( **…** )
  2. Italic ( _…_ OR *…* )
  3. Strikethrough ( ~…~ )
  4. Code Strings ( `…` )
  5. Links ( […](URL) )

Not supported formatting:

  1. Underline( __…__ )
  2. Headers ( # )
  3. Subheaders ( -# )
  4. List Items ( - OR * )
  5. Line Breaks/Multi-line ( \n OR | )
  6. Code Blocks ( ```…``` )
  7. Blockquotes ( > )
  • added a DESCRIPTION section to the workflow-drawer component that will render the unescaped markdown from the title and description for users to view the full values input when truncated in the workflow-row

Verification

Changes were tested in and verified in a dev container, running the UI and submitting workflows with a variety of title/description combinations:

  • Short Title + Short Description (no markdown)
  • Short Title + Short Description (with markdown)
  • Long Title + Long Description (no markdown)
  • Long Title + Long Description (with markdown)
  • Title Only (short/long with/without markdown)
  • Description Only (short/long with/without markdown)

image
image


UPDATE:

This formatting logic and component rendering has been added to support the recent changes to CronWorkflows, WorkflowTemplates, and ClusterWorkflowTemplates

  • Worthwhile considerations for follow-up work:
    • add drawer functionality to the workflow-templates, cluster-workflow-templates, and cron-workflows, to display INFO and show any user-added markdown
    • Tooltip on-hover behavior for quick view of truncated title or description

@stevenbjohnson stevenbjohnson force-pushed the fix/workflow-row-styles branch from e19d707 to 52a3791 Compare November 23, 2024 09:17
@stevenbjohnson stevenbjohnson marked this pull request as ready for review November 23, 2024 09:22
@stevenbjohnson stevenbjohnson marked this pull request as draft November 23, 2024 10:05
@stevenbjohnson stevenbjohnson marked this pull request as ready for review November 23, 2024 18:34
@stevenbjohnson stevenbjohnson marked this pull request as draft November 23, 2024 21:40
Signed-off-by: stevenbjohnson <[email protected]>
@stevenbjohnson stevenbjohnson marked this pull request as ready for review November 23, 2024 22:17
@stevenbjohnson stevenbjohnson changed the title fix(ui): improve workflow-row styles fix(ui): improve markdown styles for workflow-row, workflow-templates, cluster-workflow-templates, and cron-workflows Nov 25, 2024
Signed-off-by: stevenbjohnson <[email protected]>
@stevenbjohnson stevenbjohnson marked this pull request as draft November 26, 2024 00:34
@stevenbjohnson stevenbjohnson marked this pull request as ready for review November 27, 2024 03:58
Copy link
Member

@jmeridth jmeridth left a comment

Choose a reason for hiding this comment

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

a couple nits and a question about DRYing up some repetitive code. Not sure if possible to centralize or if we should.

And we discussed alt tags for mouseover when title/description gets auto-shortened due to overflow. You mentioned it makes the UI very wonky. Follow up for a future PR.

Nice work.

ui/src/cron-workflows/cron-workflow-row.scss Outdated Show resolved Hide resolved
ui/src/workflow-templates/workflow-template-row.scss Outdated Show resolved Hide resolved
ui/src/cron-workflows/cron-workflow-row.tsx Outdated Show resolved Hide resolved
ui/src/workflow-templates/workflow-template-row.tsx Outdated Show resolved Hide resolved
Copy link
Member

@Adrien-D Adrien-D left a comment

Choose a reason for hiding this comment

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

Locally tested and looks great

const title = wf.metadata.annotations?.[ANNOTATION_TITLE] ?? wf.metadata.name;
const description = (wf.metadata.annotations?.[ANNOTATION_DESCRIPTION] && `\n${wf.metadata.annotations[ANNOTATION_DESCRIPTION]}`) || '';
const hasAnnotation = title !== wf.metadata.name || description !== '';
const title = (wf.metadata.annotations?.[ANNOTATION_TITLE] && `${escapeInvalidMarkdown(wf.metadata.annotations[ANNOTATION_TITLE])}`) ?? wf.metadata.name;
Copy link
Member

Choose a reason for hiding this comment

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

Why using ?? instead of || here ? You have a chance of having an empty title

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, @Adrien-D! The syntax for the title was taken from an existing line for setting the title in the workflows-row.tsx file. Seems the use of the nullish coalescing operator is to ensure users can enter values that would otherwise be evaluated as "false-y." This approach was carried over to ensure that defaulting to the wf.metadata.name is only in the case of a null or undefined value in the [ANNOTATION_TITLE] field.

Please let me know if the use of the || operator is a preferred method to use here and I can go through the files and make the update.

Copy link
Member

Choose a reason for hiding this comment

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

There is not really a preferred method, but as it's taken from an existing line, it's all good !

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Thank you!

@terrytangyuan terrytangyuan merged commit 231d548 into argoproj:main Dec 10, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants