Skip to content

Conversation

@rj-wowza
Copy link
Contributor

Fixes issues with the build and deployment to github pages

Comment on lines 2 to +7
on:
push:
pull_request:
branches:
- main
types:
- closed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed so that PR must be merged to kick off deployment.

Comment on lines +39 to +42
- name: Upload demo page
uses: actions/upload-pages-artifact@v3
with:
path: demo/dist/

# Deploy job
deploy:
# Add a dependency to the build job
needs: build

# Grant GITHUB_TOKEN the permissions required to make a Pages deployment
permissions:
pages: write # to deploy to Pages
id-token: write # to verify the deployment originates from an appropriate source

# Specify runner + deployment step
runs-on: ubuntu-latest
steps:
path: './demo/public'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix missing Github Pages

Comment on lines +46 to +49
# - name: Publish to NPMJS
# uses: JS-DevTools/npm-publish@v3
# with:
# token: ${{ secrets.NPM_TOKEN }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is ready but want to limit the amount of change if there is trouble

Comment on lines -34 to -37
# Grant GITHUB_TOKEN the permissions required to make a Pages deployment
permissions:
pages: write # to deploy to Pages
id-token: write # to verify the deployment originates from an appropriate source
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Combined into a single job to simplify troubleshooting and moved permissions to the top

@rj-wowza rj-wowza merged commit 21c196b into main Feb 13, 2025
1 check passed
@rj-wowza rj-wowza deleted the refactor/fa-722/update-gha-build-and-deploy branch February 13, 2025 01:22
"strict": true,
"outDir": "./lib",
"moduleResolution": "node"
"target": "es2015",
Copy link
Contributor

Choose a reason for hiding this comment

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

if you want to, I think we could bump this up to something more modern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i figured it was better to change as little as possible in case there was user impact. i don't want to wreck some IE9 user's day without at least a deprecation warning

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that is fair, but flowplayer doesn't support IE, though we do support some very old iOS versions

Copy link
Contributor Author

@rj-wowza rj-wowza Feb 13, 2025

Choose a reason for hiding this comment

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

ah i see then it doesn't matter what my output target is, it will be chained to the "native" player's target 👍 we should sync those targets then

Copy link
Contributor

Choose a reason for hiding this comment

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

we use es2017 there for now.

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