Skip to content

chore: Address TiCS coding standards warnings for base JS#5104

Merged
steverydz merged 1 commit intomainfrom
TiCS-coding-standards-base-js
Apr 22, 2025
Merged

chore: Address TiCS coding standards warnings for base JS#5104
steverydz merged 1 commit intomainfrom
TiCS-coding-standards-base-js

Conversation

@steverydz
Copy link
Copy Markdown
Contributor

@steverydz steverydz commented Apr 22, 2025

Done

Addressed TiCS coding standards warnings for base JS

  • Converted the parameters to an object for the triggerEvent function to meet the parameter limit requirement, which makes it clearer what each argument is for when using the function
  • Added missing return types to functions
  • Marked DOM selector elements as possibly undefined and added appropriate checks for their existence
  • Renamed origin to urlLocation as origin is an existing global object
  • Removed some unnecessary if conditions
  • Fixed some basic linting errors

How to QA

All JS tests and linter checks should pass

Testing

  • This PR has tests
  • No testing required (explain why): No behavioural changes

@webteam-app
Copy link
Copy Markdown

@steverydz steverydz changed the title chore: Address TiCS codings standards warnings for base JS chore: Address TiCS coding standards warnings for base JS Apr 22, 2025
Comment thread static/js/base/contactForm.ts Dismissed
@codeEmpress1
Copy link
Copy Markdown
Contributor

LGTM!

@steverydz steverydz merged commit 5573f80 into main Apr 22, 2025
11 checks passed
@steverydz steverydz deleted the TiCS-coding-standards-base-js branch April 22, 2025 10:04
Comment thread static/js/base/ga.ts
}

target = eventTarget.closest("a") as HTMLAnchorElement;
target = eventTarget.closest("a")!;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not exactly sure if I understand the ! usage here.
We are telling TS, that this is not null (if I understand it correctly), but next line we actually have a conditional statement to check if it's not null (because then we look for a button instead).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bartaz Actually that makes sense - I meant to revert that change as I did on others on closer review. TiCS wants us to mark every selector as not null it seems, even though we can't always be sure about that. I'll fix that in the next PR.

Copy link
Copy Markdown
Member

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

LGTM, with a question (in inline comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants