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

Update microsoft-clarity.njk #480

Merged
merged 1 commit into from
Mar 6, 2022
Merged

Update microsoft-clarity.njk #480

merged 1 commit into from
Mar 6, 2022

Conversation

hushanjushi
Copy link
Contributor

update the microsoft clarity inline script

PR Checklist

  • The commit message follows guidelines for NexT.
  • Tests for the changes was maked (for bug fixes / features).
    • Muse | Mist have been tested.
    • Pisces | Gemini have been tested.
  • Docs in NexT website have been added / updated (for features).

PR Type

  • Bugfix.
  • Feature.
  • Code style update (formatting, local variables).
  • Refactoring (no functional changes, no api changes).
  • Documentation.
  • Translation.
  • Other... Please describe:

What is the current behavior?

Normal as we look like.
Issue resolved:

What is the new behavior?

Microsoft clarity works well.

  • Link to demo site with this changes:
  • Screenshots with this changes:

How to use?

In NexT _config.yml:

update the microsoft clarity inline script
@welcome
Copy link

welcome bot commented Mar 5, 2022

Thanks so much for opening your first PR here!

@CLAassistant
Copy link

CLAassistant commented Mar 5, 2022

CLA assistant check
All committers have signed the CLA.

@stevenjoezhang
Copy link
Member

See also #451
CC @wasi-master

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1937386099

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.468%

Totals Coverage Status
Change from base Build 1915994541: 0.0%
Covered Lines: 138
Relevant Lines: 141

💛 - Coveralls

@hushanjushi
Copy link
Contributor Author

See also #451 CC @wasi-master

I see, but just a little diffenrent. It can't be minified.( It will cause a bug that clarity dashboard is empty...

@hushanjushi
Copy link
Contributor Author

See also #451 CC @wasi-master

I see, but just a little diffenrent. It can't be minified.( It will cause a bug that clarity dashboard is empty...

not about minified.js, just about inline script in output

@wasi-master
Copy link
Contributor

wasi-master commented Mar 6, 2022

I see, but just a little different. It can't be minified. ( It will cause a bug that clarity dashboard is empty...

Oh, my bad, I didn't test this on an actual project. I thought minifying it would not change any behavior. I just did it like that to keep it consistent with the rest of the analytics providers. But I guess you need to use the exact code Microsoft provides

@stevenjoezhang You can accept this PR. And sorry for the inconvenience

@hushanjushi
Copy link
Contributor Author

I see, but just a little different. It can't be minified. ( It will cause a bug that clarity dashboard is empty...

Oh, my bad, I didn't test this on an actual project. I thought minifying it would not change any behavior. I just did it like that to keep it consistent with the rest of the analytics providers. But I guess you need to use the exact code Microsoft provides

@stevenjoezhang You can accept this PR. And sorry for the inconvenience

Emm, the change is that the njk will use the exact code Microsoft provides. So could you give this pull request a milestone....?

@hushanjushi
Copy link
Contributor Author

Is there anyone give the milestone to this pull? I don't have the permission......

@sli1989
Copy link

sli1989 commented Mar 6, 2022

Is there anyone give the milestone to this pull? I don't have the permission......

It would be tagged when merged.

@stevenjoezhang stevenjoezhang merged commit 358a3c0 into next-theme:master Mar 6, 2022
@welcome
Copy link

welcome bot commented Mar 6, 2022

Congrats on merging your first pull request here! 🎉 How awesome!

@stevenjoezhang stevenjoezhang added this to the 8.10.2 milestone Mar 6, 2022
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.

6 participants