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

refactor(ui): optimize Link functionality #11743

Merged
merged 3 commits into from
Sep 30, 2023

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Sep 4, 2023

Motivation

  • Optimize the links module and its usage a bit
  • Simplify code a bit by reducing nesting
  • Fix some JS conventions etc

Stumbled upon this while investigating #11741 and thought it could use some improvements

Modifications

  • move static functions outside of the component

    • this way they are not created every time the component is and hence use less memory etc
  • use named functions instead of consts assigned to anonymous functions

    • this makes tracing a bit easier and source maps a bit clearer
    • (also technically a memory improvement as well)
  • both of these also reduce nesting

  • re-use openLinkWithKey in WorkflowDetails instead of having duplicate code

    • rename to openLinkWithKey so that it doesn't overlap with openLink and to be a bit more specific
    • (also a very tiny memory improvement)
  • remove redundant formatURL function

  • use lowercase "p" in processURL

    • JS convention that only classes are capitalized (or components in React, which used to always be classes)
      • I imagine this might have been capitalized b/c Go uses capitals for exports?
  • be more specific with urlExpression as the argument since it is not necessarily a raw URL and that is why it needs "processing"

Verification

Stylistic and performance changes, but no usage changes (minor semantic differences that improve tracing, memory, etc as mentioned above, but do not change usage).

- move static functions outside of the component
  - this way they are not created every time the component is and hence use less memory etc
- use named `function`s instead of `const`s assigned to anonymous functions
  - this makes tracing a bit easier and source maps a bit clearer
  - (also technically a memory improvement as well)

- re-use `openLinkWithKey` in `WorkflowDetails` instead of having duplicate code
  - rename to `openLinkWithKey` so that it doesn't overlap with `openLink` and to be a bit more specific
  - (also a very tiny memory improvement)

- remove redundant `formatURL` function

- use lowercase "p" in `processURL`
  - JS convention that only classes are capitalized (or components in React, which used to always be classes)
    - I imagine this might have been capitalized b/c Go uses capitals for exports?

- be more specific with `urlExpression` as the argument since it is not necessarily a raw URL and that is why it needs "processing"

Signed-off-by: Anton Gilgur <[email protected]>
Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

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

Could totally be my lack of frontend knowledge, but I didn't quite understand the function name for openLinkWithKey here, I assumed this was more my fault and approved.

@agilgur5
Copy link
Contributor Author

agilgur5 commented Sep 15, 2023

I didn't quite understand the function name for openLinkWithKey here

I wrote this a few weeks ago, but it's quite literal, since it's when clicked with ctrlKey or metaKey (i.e. Ctrl+Click or Command+Click) to open in a new tab. "WithKey" as in with one of those two keys (or without)

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.

This doesn't seem to fix #11741?

@agilgur5
Copy link
Contributor Author

agilgur5 commented Sep 26, 2023

This doesn't seem to fix #11741?

Correct. I didn't say it did. Per the opening comment, this was an incidental finding while I was investigating that

@terrytangyuan terrytangyuan enabled auto-merge (squash) September 28, 2023 21:20
auto-merge was automatically disabled September 29, 2023 00:36

Head branch was pushed to by a user without write access

@agilgur5
Copy link
Contributor Author

poop, this had a one-line semi-colon conflict with #11882 😭 . mentioned on Slack that I tried pretty hard to avoid a conflict on this old PR but missed a single character

@terrytangyuan terrytangyuan merged commit a363e6a into argoproj:master Sep 30, 2023
25 checks passed
@agilgur5 agilgur5 deleted the refactor-ui-links-functions branch September 30, 2023 04:02
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.

3 participants