Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Migrate highlight.js API Call Usage in TextualBody.tsx #9989

Closed
wants to merge 12 commits into from
Closed

Migrate highlight.js API Call Usage in TextualBody.tsx #9989

wants to merge 12 commits into from

Conversation

mustafa-kapadia1483
Copy link
Contributor

@mustafa-kapadia1483 mustafa-kapadia1483 commented Jan 25, 2023

Checklist

  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Type: Task
Updated highlight usage to use highlight(code, options) instead as highlight(lang, code, ...args) has been deprecated
fixes Update highlight.js usage in-code #22233

Signed-off-by: Mustafa Kapadia mustafakapadia76@gmail.com


This change is marked as an internal change (Task), so will not be included in the changelog.

Updated highlight usage to  use highlight(code, options) instead as highlight(lang, code, ...args) has been deprecated
Updated highlight usage to  use highlight(code, options) instead as highlight(lang, code, ...args) has been deprecated

Signed-off-by: Mustafa Kapadia <mustafakapadia76@gmail.com>
@mustafa-kapadia1483 mustafa-kapadia1483 requested a review from a team as a code owner January 25, 2023 18:01
@github-actions github-actions bot added Z-Community-PR Issue is solved by a community member's PR T-Task Refactoring, enabling or disabling functionality, other engineering tasks labels Jan 25, 2023
@mustafa-kapadia1483 mustafa-kapadia1483 changed the title Updated highlight.js API Call Usage Migrate highlight.js API Call Usage in TextualBody.tsx Jan 26, 2023
src/components/views/messages/TextualBody.tsx Outdated Show resolved Hide resolved
src/components/views/messages/TextualBody.tsx Outdated Show resolved Hide resolved
@dbkr
Copy link
Member

dbkr commented Jan 26, 2023

Also, I don't think you need a 'Notes' tag in this PR: it's not a user-facing change so it doesn't need to be in the changelog (the explanation is useful though, so I'd keep it in the description, just not as a changelog note).

early exit in case code text content is empty & removed initialization of advertisedLang as empty string
@mustafa-kapadia1483
Copy link
Contributor Author

Thank You @dbkr for reviewing the changes, I have made the changes as per the review

if (code.textContent.length > MAX_HIGHLIGHT_LENGTH) {
const codeTextContent = code.textContent;
if (!codeTextContent) {
console.log("Code block is empty");
Copy link
Member

Choose a reason for hiding this comment

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

We use the loglevel library for logging, so no console.log statements, although I don't think a log line is necessary here - it could get quite noisy.

Copy link
Contributor Author

@mustafa-kapadia1483 mustafa-kapadia1483 Jan 27, 2023

Choose a reason for hiding this comment

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

@dbkr there was a console.log when codeTextContent.length > MAX_HIGHLIGHT_LENGTH hence I thought same process should be replicated when exiting early for empty string / null

ref:

if (codeTextContent.length > MAX_HIGHLIGHT_LENGTH) {
    console.log(
        "Code block is bigger than highlight limit (" +
            codeTextContent.length +
            " > " +
            MAX_HIGHLIGHT_LENGTH +
            "): not highlighting",
    );
    return;
}

Should both console.logs be removed then ?

@t3chguy
Copy link
Member

t3chguy commented Jan 30, 2023

Duplicate of #9923

@t3chguy t3chguy marked this as a duplicate of #9923 Jan 30, 2023
@t3chguy t3chguy closed this Jan 30, 2023
@mustafa-kapadia1483
Copy link
Contributor Author

Duplicate of #9923

@t3chguy, this PR was fixing the static Static Analysis / Typescript Strict Error Checker (--strict --noImplicitAny) error in #9923

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update highlight.js usage in-code
3 participants