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

Create "lite" version (js only, without CSS) #269

Merged
merged 13 commits into from
Jul 8, 2024
Merged

Conversation

egirard
Copy link
Collaborator

@egirard egirard commented May 30, 2024

Also changes test for CSS support to look for view-timeline

Copy link
Owner

@flackr flackr left a comment

Choose a reason for hiding this comment

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

I think it's just churn in the wpt repo, but it looks like there's two fewer tests run than expected: https://github.com/flackr/scroll-timeline/actions/runs/9304178220/job/25608156082?pr=269

Can you update expected.txt?

src/scroll-timeline-css.js Outdated Show resolved Hide resolved

export function initPolyfill() {
// Don't load if browser claims support
if (CSS.supports("view-timeline")) {
Copy link
Owner

Choose a reason for hiding this comment

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

For the JS polyfill, feature detection should look for 'ViewTimeline' in window or window.ViewTimeline !== undefined to see if the javascript interface has been implemented by the browser.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

animate,
elementGetAnimations,
documentGetAnimations,
ProxyAnimation
Copy link
Owner

Choose a reason for hiding this comment

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

nit: We're not using any of these imports here - they can be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

},
}
},
})
Copy link
Owner

Choose a reason for hiding this comment

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

This is duplicating a lot of config that likely should be kept in sync. Can you move all of this to a file like vite.common.js, e.g.

export function buildConfig(source, filename) {
    return {
        build: {
            sourcemap: true,
            emptyOutDir: false,
            lib: {
            // Could also be a dictionary or array of multiple entry points
            entry: source,
            name: 'ScrollTimeline',
            // the proper extensions will be added
            fileName: (format, entryAlias) => `${filename}${format=='iife'?'':'-' + format}.js`,
            formats: ['iife'],
            },
            minify: 'terser',
            terserOptions: {
            keep_classnames: /^((View|Scroll)Timeline)|CSS.*$/
            },
            rollupOptions: {
            output: {
                // Provide global variables to use in the UMD build
                // for externalized deps
                globals: {
                },
            },
            }
        }
    };
}

vite.config.js:

import { resolve } from 'path'
import { defineConfig } from 'vite'
import { buildConfig } from './vite.common'

export default defineConfig(buildConfig(resolve(__dirname, 'src/index.js'), 'scroll-timeline'));

vite.config.lite.js:

import { resolve } from 'path'
import { defineConfig } from 'vite'
import { buildConfig } from './vite.common'

export default defineConfig(buildConfig(resolve(__dirname, 'src/index-lite.js'), 'scroll-timeline-lite'));

This also eliminates the need to have a separate dist dir by configuring vite to not empty the dir on building.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

// initCSSPolyfill returns true iff the host browser supports SDA
console.debug("Polyfill initializing.");
if (initCSSPolyfill()) {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm a bit unsure about this. If the browser supports the CSS API's but not the JS ones maybe we should still polyfill the JS apis.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Best to check with typeof ScrollTimeline !== "undefined" (and ViewTimeline) here indeed.

@egirard
Copy link
Collaborator Author

egirard commented Jun 20, 2024

Updated code to address feedback - thanks.
Note that the css feature detection is now removed and I'll reland in a new patch.

Copy link
Owner

@flackr flackr 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 with a few nits.

.gitignore Outdated Show resolved Hide resolved
src/index-lite.js Outdated Show resolved Hide resolved
@@ -169,7 +169,7 @@
<input type="checkbox" name="timeline-insets" id="timeline-insets">
<label for="timeline-insets">Use inset 100px 200px</label>
</body>
<script src="../../dist/scroll-timeline.js"></script>
<script src="../../dist-lite/scroll-timeline-lite.js"></script>
Copy link
Owner

Choose a reason for hiding this comment

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

this is just dist/scroll-timeline-lite.js now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks - fixed.

demo was linked to the wrong directory.... now fixed;
Typo in copyright date... also fixed.
This is only to address a flakey test, afaik.
@egirard egirard merged commit 0a3a758 into flackr:master Jul 8, 2024
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