-
Notifications
You must be signed in to change notification settings - Fork 95
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
Conversation
Also changes test for CSS support to look for view-timeline
There was a problem hiding this 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/init-polyfill.js
Outdated
|
||
export function initPolyfill() { | ||
// Don't load if browser claims support | ||
if (CSS.supports("view-timeline")) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/index-lite.js
Outdated
animate, | ||
elementGetAnimations, | ||
documentGetAnimations, | ||
ProxyAnimation |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
vite.config.lite.js
Outdated
}, | ||
} | ||
}, | ||
}) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Plus other cleanup from review.
Updated code to address feedback - thanks. |
There was a problem hiding this 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.
demo/view-timeline/index.html
Outdated
@@ -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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - fixed.
This is only to address a flakey test, afaik.
Also changes test for CSS support to look for view-timeline