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

Added CI workflow with a job to format the code on the PR #50

Merged
merged 13 commits into from
Feb 14, 2024

Conversation

hsein-bitar
Copy link
Contributor

Asana

https://app.asana.com/0/1202852195727075/1205992270273982/f

Context

Added CI workflow with a job to format the code on the PR

@hsein-bitar
Copy link
Contributor Author

hsein-bitar commented Feb 13, 2024

@tylerdigital @NatalieMac hello, this should be ready.
The PR includes lots of file changes introduced by the CI pipeline formatting many files.
Outside of the formatting, the main changes can be viewed using this link, with some JS formatting to be ignored:

  • We introduced ci.yml workflow, which runs only on pull requests, and attempts to format and catch any code errors
  • The workflow relies on some configuration files, so I also added .prettierignore, .eslintrc.js, and composer files

Testing

I tested a Draw Attention element that I had previously setup:

  • confirmed it can still be viewed as expected ✅
  • confirmed it still has the settings and can be edited as expected ✅
  • added new hotspots to it and confirmed it can be updated with no issues ✅

I also tested replacing the plugin with a new build, and confirmed it works ✅

@hsein-bitar hsein-bitar marked this pull request as ready for review February 13, 2024 16:42
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we're attempting to un-minify this file. Is there a way to just exclude this file?

Copy link
Contributor Author

@hsein-bitar hsein-bitar Feb 14, 2024

Choose a reason for hiding this comment

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

Oh sure, thanks for catching this. I had excluded all files matching min.* but missed this one. Just excluded the file from formatting now

@NatalieMac
Copy link
Contributor

I looped in Cyndy to do some QA just in case.

Copy link
Collaborator

@tylerdigital tylerdigital left a comment

Choose a reason for hiding this comment

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

@cynhu92 @gnarza please fully try out this version of Draw Attention to make sure we haven't accidentally broken anything. This shouldn't affect anything, but we're modifying a lot of files throughout the codebase

Copy link
Contributor

@cynhu92 cynhu92 left a comment

Choose a reason for hiding this comment

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

I was testing this, but it seems to be only DA Free, right? Do we have a Pro version to test with?

@cynhu92 cynhu92 self-requested a review February 14, 2024 22:14
Copy link
Contributor

@cynhu92 cynhu92 left a comment

Choose a reason for hiding this comment

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

Was doing my testing here: https://pr50-drawattention-cyn.sandbox.ssa.rocks/test/

Tested:

  • Creating image ✅
  • Deleting image ✅
  • Replacing image ✅
  • Creating hotspots ✅
  • Deleting hotspots ✅
  • Editing hotspots ✅
  • Using polygon, rectangle, circle hotspots ✅
  • Adding shortcodes and links to hotspots ✅
  • Adding videos ✅
  • Using Go to Url and More info ✅
  • Changing hotspot styles and borders ✅

@NatalieMac
Copy link
Contributor

I was testing this, but it seems to be only DA Free, right? Do we have a Pro version to test with?

Draw Attention free and Pro are two totally separate repositories (unlike SSA where all versions are in one repo), so they have to be updated and tested separately. @cynhu92

Copy link
Contributor

@gnarza gnarza left a comment

Choose a reason for hiding this comment

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

Tested on sandbox:
Adding main image ✅
Creating hotspot polygon ✅
Creating hotspot square ✅
Creating hotspot circle ✅
Editing hotspots ✅
Zooming and panning ✅
Deleting hotspots ✅
Show description action ✅
Go to URL action ✅
Attach Detail Image ✅
Highlight Styling ✅
More Info Box Styling ✅
Apply Color Scheme ✅

Tested on fresh local:
Adding main image ✅
Creating hotspot polygon ✅
Creating hotspot square ✅
Creating hotspot circle ✅
Editing hotspots ✅
Zooming and panning ✅
Deleting hotspots ✅
Show description action ✅
Go to URL action ✅
Attach Detail Image ✅
Highlight Styling ✅
More Info Box Styling ✅
Apply Color Scheme ✅

@hsein-bitar hsein-bitar merged commit 91411fe into master Feb 14, 2024
14 checks passed
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.

None yet

6 participants