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

feat/blog-post-scripts #823

Merged
merged 6 commits into from
Jan 16, 2025
Merged

feat/blog-post-scripts #823

merged 6 commits into from
Jan 16, 2025

Conversation

WesselSmit
Copy link
Contributor

What changes were made

  • enable scripts from the cms to run in blog posts
  • added custom-script component
  • added noscript warning to help users with javascript disabled understand that the page may not function as explained in the blog post

Ticket

https://trello.com/c/SJizgBYO/753-as-a-writer-i-want-to-add-javascript-to-my-blog-post-so-i-can-have-interactive-examples

How to test or check results

  • check deploy preview: in my branch, the 'scroll-driven-animations-for-better-transitions' blog post (in 'en' locale) has a script that will log something to the console and shows a noscript if JS is disabled

Checks

@WesselSmit WesselSmit self-assigned this Aug 30, 2024
@WesselSmit WesselSmit changed the title feat: enable scripts to run in blog posts feat/blog-post-scripts Aug 30, 2024
Copy link
Contributor

@jurgenbelien jurgenbelien left a comment

Choose a reason for hiding this comment

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

Looks good! Tried it out with my own blog post and it seems to work as expected. Even the dato field grows when I add more code!

I do wonder if the nesting of the css and noscript can be improved, even though it does not seem to be influencing document flow

migrations/1725011361_addScriptFieldToBlogPosts.ts Outdated Show resolved Hide resolved
@jurgenbelien jurgenbelien force-pushed the feat/blog-post-scripts branch from d6de013 to 1b62fe0 Compare January 10, 2025 12:03
Copy link
Member

@Siilwyn Siilwyn left a comment

Choose a reason for hiding this comment

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

Looks good, two small comments!

<template>
<div class="custom-script">
<!-- we show a <noscript> element if the user has javascript disabled, this is necessary because some functionality might be broken due to javascript being disabled and its important that users are notified why it isn't working -->
<!-- this needs to be v-html instead of your typical {{ someValue }} interpolation because of a bug that content in noscript does not seem to be evaluated by Vue -->
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is a bug, feels like intended behaviour? Maybe we can just mention it's not being evaluated?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, not a bug!

if (props.unmountScript) {
const unmountScript = runScriptInBrowser(props.unmountScript, onUnmountScriptId);

// how to ensure the unmountScript has a chance to be executed in the browser?
Copy link
Member

Choose a reason for hiding this comment

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

Since runScriptInBrowser is sync this shouldn't be an issue right?
Could add nextTick before removal to give it some extra time.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was our assumption as well, will remove

@jurgenbelien jurgenbelien force-pushed the feat/blog-post-scripts branch from 35c8ccf to bedfc58 Compare January 15, 2025 10:42
@jurgenbelien jurgenbelien merged commit 33be5c2 into main Jan 16, 2025
4 checks passed
@jurgenbelien jurgenbelien deleted the feat/blog-post-scripts branch January 16, 2025 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants