-
Notifications
You must be signed in to change notification settings - Fork 229
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
chore: screenshots to be dynamically added in prs #1233
base: flutter_app
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request dynamically generates screenshot tables in the pull request comment workflow based on the files present in the Sequence diagram for generating screenshot HTMLsequenceDiagram
participant GitHub Actions Workflow
participant File System
GitHub Actions Workflow->>File System: Check if 'pr-screenshots' directory exists
alt Directory exists
File System-->>GitHub Actions Workflow: Returns true
GitHub Actions Workflow->>File System: Read files in 'pr-screenshots' directory
File System-->>GitHub Actions Workflow: Returns list of files
loop For each file
GitHub Actions Workflow->>GitHub Actions Workflow: Match filename against regex
alt Filename matches
GitHub Actions Workflow->>GitHub Actions Workflow: Extract device name
GitHub Actions Workflow->>GitHub Actions Workflow: Add file to deviceScreenshots object
end
end
GitHub Actions Workflow->>GitHub Actions Workflow: Generate HTML table from deviceScreenshots
else Directory does not exist
File System-->>GitHub Actions Workflow: Returns false
end
GitHub Actions Workflow->>GitHub Actions Workflow: Update PR comment with generated HTML
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @adityastic - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding error handling for file system operations in the GitHub script.
- The screenshot table generation could be simplified by using a single loop and calculating the number of columns dynamically.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Build StatusBuild successful. APKs to test: https://github.com/fossasia/badgemagic-app/actions/runs/13736940703/artifacts/2715616643. Screenshots (Android)
Screenshots (iPhone)
Screenshots (iPad)
|
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.
@adityastic Looks good to me ! I am not able to imagine as to how the screenshots would look though. Let's maybe merge and try ?
d14b7e1
to
381b8c8
Compare
Summary by Sourcery
Dynamically generate screenshot tables in pull request comments based on files found in the 'screenshots' directory. Also, update the screenshot file naming convention to use hyphens instead of underscores.
CI: