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

fix: Promo Video updates #594

Merged
merged 11 commits into from
Oct 17, 2023
Merged

fix: Promo Video updates #594

merged 11 commits into from
Oct 17, 2023

Conversation

AndyEPhipps
Copy link
Contributor

@AndyEPhipps AndyEPhipps commented Oct 12, 2023

PR description

What is it doing?

Some additions and simplification to make the new functionality better fit for purpose

Why is this required?

Good content = nice

link to Jira ticket:

No ticket, just an extension of the original rly

Quick Checklist:

  • My PR title follows the Conventional Commit spec.

  • I have filled out the PR description as per the template above.

  • I have added tests to cover new or changed behaviour. to follow in a separate PR

  • I have updated any relevant documentation.

Important! - lastly, make sure to squash merge...

@AndyEPhipps AndyEPhipps self-assigned this Oct 12, 2023
@AndyEPhipps
Copy link
Contributor Author

AndyEPhipps commented Oct 17, 2023

Showing how pre-existing non-video Promos haven't been affected:

https://deploy-preview-594--cr-component-library.netlify.app/#!/Promo/1
https://cr-component-library.netlify.app/#!/Promo/1

@AndyEPhipps AndyEPhipps removed the request for review from michael-odonovan October 17, 2023 09:28
@AndyEPhipps
Copy link
Contributor Author

Sorry Mike, found a few issues; will reassign for review once it's actual sorted :/

@AndyEPhipps
Copy link
Contributor Author

@KrupaPammi, any idea what's happening with Playwright here? I wasn't aware we had any PW testing in Component Library, plus I can't actually see any PW processes running under Actions so this just seems to be blocking my work :/

@KrupaPammi
Copy link
Contributor

Hi @AndyEPhipps, it's part of this PR here that I had to park to look into FSU and Donate tests failing issues. I had to add playwright workflow to run on Github actions but will disable it now. So, the change of cypress to playwright is due to intermittent cypress test failures and some outdated tests. What I thought was to add playwright tests which are fast, and add only functionality-related tests instead of every single component, as they are already covered in Jest tests. Please let me know if this is of any concern.

@AndyEPhipps
Copy link
Contributor Author

Hi @AndyEPhipps, it's part of this PR here that I had to park to look into FSU and Donate tests failing issues. I had to add playwright workflow to run on Github actions but will disable it now. So, the change of cypress to playwright is due to intermittent cypress test failures and some outdated tests. What I thought was to add playwright tests which are fast, and add only functionality-related tests instead of every single component, as they are already covered in Jest tests. Please let me know if this is of any concern.

Ah right okay, thank you for the clarification; that all sounds sensible to me.

What do you think would be the timeline for that work, do you have much more to do in that PR? Would it just be possible to remove the branch protection rule for now without it disrupting your work?

I guess in the worst case, we just have to force-merge these PRs while you're working on the PW stuff.. I'm not sure if that'll stop the NPM Publish job happening here or not, but I'll give it a go now!

@KrupaPammi
Copy link
Contributor

@AndyEPhipps I have now removed the PW workflow from GitHub Actions.

@AndyEPhipps
Copy link
Contributor Author

Thanks @KrupaPammi, but do let me know if this just makes your life more difficult in terms of running and confirming the tests you're developing actually outside of your local?

@AndyEPhipps AndyEPhipps merged commit 4a0eb76 into master Oct 17, 2023
8 checks passed
@AndyEPhipps AndyEPhipps deleted the promo_video_updates branch October 17, 2023 11:12
@KrupaPammi
Copy link
Contributor

Hi @AndyEPhipps, it's part of this PR here that I had to park to look into FSU and Donate tests failing issues. I had to add playwright workflow to run on Github actions but will disable it now. So, the change of cypress to playwright is due to intermittent cypress test failures and some outdated tests. What I thought was to add playwright tests which are fast, and add only functionality-related tests instead of every single component, as they are already covered in Jest tests. Please let me know if this is of any concern.

Ah right okay, thank you for the clarification; that all sounds sensible to me.

What do you think would be the timeline for that work, do you have much more to do in that PR? Would it just be possible to remove the branch protection rule for now without it disrupting your work?

I guess in the worst case, we just have to force-merge these PRs while you're working on the PW stuff.. I'm not sure if that'll stop the NPM Publish job happening here or not, but I'll give it a go now!

@AndyEPhipps I can work on those PW tests locally, as I now know the setup on GitHub actions is working fine so keeping the PW workflow on branch rules disabled should be fine until the tests are ready to be reviewed and merged. By this, none of the PRs or merging will be affected.

@AndyEPhipps
Copy link
Contributor Author

Okay brilliant, thank you!

@github-actions
Copy link

🎉 This PR is included in version 7.22.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants