Skip to content

Conversation

@agilgur5
Copy link

@agilgur5 agilgur5 commented Jan 28, 2024

Follow-up to #10553 issues

Related / partial fix for to #12059, although the PR was merged after that issue was created

Motivation

  • react-markdown and remark-gfm are large deps, so we should code-split / lazy load them

    • and we should only load them if a user is using this feature
      • can't quite detect if a string is markdown without a dep, but can at least check if the annotations are used
  • CSS had several issues

    • was imported from the wrong component
    • overlay was not sufficiently unique (as we currently use global CSS and not something like CSS Modules)
    • some unnecessary CSS attributes
    • most importantly, the positioning of rows broke and did not flex to the last column
      • it caused columns to not match the heading very confusingly and a lot of empty space on the right as well
      • that also broke some overflow properties (it re-broke fix(ui): ensure WorkflowsRow message is not too long #11908)
      • and it broke the details drawer as well
        • I couldn't get it to open half the time
        • when I did open it, the first two columns were empty, and with the last few columns empty too, it looked like it was just centered horizontally (unintentionally)
  • fix nested link issues

    • overlay was unintuitive and hacky
      • can instead use JS to capture the event & explicitly prevent bubble up
        • semantically, the JS is still hacky to have nested links, but it less hacky
          • nested linking is also an optional feature that happens only if a user has an annotation, with embedded markdown, and with a link in the markdown (i.e. an optional feature of an optional feature of an optional feature), so I don't mind that hack as much now
  • remove Link for the entire row as it was unintuitive, especially with the drawer

    • the drawer click conflicted, but superseded the link click
    • the whole drawer would be highlighted on hover since the whole row was a link, including the drawer
    • one could also not copy+paste things from the row because it caused a click
      • in particular, there were several times I wanted to copy something from the details drawer and would instead end up on the details page
    • only link the workflow name now

Modifications

  • code-split react-markdown and remark-gfm
    • only load them when annotations are present
  • revert the split of the workflows-row-name.tsx file and merge it back to where it was
    • import the CSS from workflows-row.tsx, not workflows-row-name.tsx
      • remove most of the CSS as the Link was changed and because some of it was not necessary
  • fix several issues with the links
    • the overlay anchor element was a workaround for nested linking (which could optionally be used with the markdown feature), remove it and use JS to handle nested links instead
      • can explicitly control event bubbling in JS
      • the overlay caused problems with the drawer and removed the highlighting that an anchor element has on hover
    • only link the name column and not the entire row (which included the drawer before, both open and closed)

Verification

Rendered UI itself

Manually tested the UI:

Before had several issues (as listed in #10553 (review)):

Misplaced columns; row does not flex all the way to the right:
Screenshot 2024-01-22 at 1 05 49 AM

Details drawer has empty space on both left and right:
Screenshot 2024-01-28 at 12 21 52 PM
Screenshot 2024-01-28 at 12 22 46 PM

Overflow that was fixed in #11908 was re-broken:
Screenshot 2024-01-28 at 12 23 53 PM

After:
Screenshot 2024-01-27 at 9 26 51 PM

UI deps code-split

Shaved off ~575KB / ~140KB minified / ~41KB gzipped from the main entrypoint:
Screenshot 2024-01-28 at 1 39 49 PM
Screenshot 2024-01-28 at 1 40 01 PM

Anton Gilgur added 2 commits January 28, 2024 13:45
- `react-markdown` and `remark-gfm` are large deps, so we should code-split / lazy load them
  - and we should only load them if a user is using this feature
    - can't quite detect if a string is markdown without a dep, but can at least check if the annotations are used
  - shaved off ~575KB / ~140KB minified / ~41KB gzipped from the main entrypoint

- CSS had several issues
  - was imported from the wrong component
  - `overlay` was not sufficiently unique (as we currently use global CSS and not something like CSS Modules)
  - some unnecessary CSS attributes
  - most importantly, the positioning of rows broke and did not flex to the last column
    - it caused columns to not match the heading very confusingly and a lot of empty space on the right as well
    - that also broke some overflow properties
    - and it broke the details drawer as well

- remove `Link` for the entire row as it was unintuitive, especially with the drawer
  - one could also not copy+paste things from the row because it caused a click
  - only link the workflow name now

- fix nested link issues
  - capture the event, prevent bubble up, and process it with JS

Signed-off-by: Anton Gilgur <[email protected]>
- these were from when I was trying to lazy load `remark-gfm` in the same file and for some reason forgot I could just split both imports into one file that I could lazy load / code-split

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

Nice use of suspense. Just one minor nit.

@juliev0 juliev0 merged commit d4ca8d9 into argoproj:main Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/build Build or GithubAction/CI issues area/ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants