-
Notifications
You must be signed in to change notification settings - Fork 3
Refactor: [fa-722] update gha build and deploy #55
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
Conversation
| on: | ||
| push: | ||
| pull_request: | ||
| branches: | ||
| - main | ||
| types: | ||
| - closed |
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.
Changed so that PR must be merged to kick off deployment.
| - 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' |
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.
Fix missing Github Pages
| # - name: Publish to NPMJS | ||
| # uses: JS-DevTools/npm-publish@v3 | ||
| # with: | ||
| # token: ${{ secrets.NPM_TOKEN }} |
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 this is ready but want to limit the amount of change if there is trouble
| # 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 |
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.
Combined into a single job to simplify troubleshooting and moved permissions to the top
| "strict": true, | ||
| "outDir": "./lib", | ||
| "moduleResolution": "node" | ||
| "target": "es2015", |
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.
if you want to, I think we could bump this up to something more modern.
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 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
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 guess that is fair, but flowplayer doesn't support IE, though we do support some very old iOS versions
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.
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
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.
we use es2017 there for now.
Fixes issues with the build and deployment to github pages