Skip to content

feat:Added visual tests for KIcon #1035

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

Merged
merged 4 commits into from
Jun 3, 2025

Conversation

AadarshM07
Copy link

@AadarshM07 AadarshM07 commented May 26, 2025

Description

Added visual testing script for KIcon . This covers visual tests for the following snapshots :

  • KIcon - profile icon (default color)
  • KIcon - profile icon (green color)
  • KIcon - allActivities icon (color prop ignored)
  • KIcon - broken image for non-existent icon

Issue addressed

Closes #1008

screenshots

The following are snapshots from percy

image
image
image
image

Changelog

  • Description: Adds visual tests for KIcon.
  • Products impact: None
  • Addresses: #1009
  • Components: No
  • Breaking: No
  • Impacts a11y: No
  • Guidance: None

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

Comments

  • I am open to make any necessary changes needed.

@AlexVelezLl
Copy link
Member

Thanks @AadarshM07! This is looking great! :D

Just a little change - could you assign a width and height values to the snapshot so it doesnt look so wide, and the icon doesnt look so small? Just like in the attached examples of KButton.

Also, in your first screenshot it seems that this is capturing an undefined error. Could you please dive a little bit into why is this error appearing? Thanks!

@AlexVelezLl AlexVelezLl self-assigned this May 27, 2025
@MisRob MisRob requested review from MisRob and removed request for MisRob May 28, 2025 02:40
@AadarshM07
Copy link
Author

Hey @AlexVelezLl ,
I’ll make the changes. About the error , I’m not sure how it appeared since it’s not happening now. But i noticed in the requirements you mentioned KIcon with the icon prop set to Non icon should show a broken image. The visual test times out for this particular snapshot because the component doesn’t actually render any fallback or broken image for a Non icon prop. I think the test failure is related to this missing fallback.

@AlexVelezLl AlexVelezLl self-requested a review May 28, 2025 15:46
Copy link
Contributor

github-actions bot commented May 28, 2025

Percy Visual Test Results

Percy Dashboard: View Detailed Report

Environment:

  • Node.js Version: 18.x
  • OS: Ubuntu-latest

Instructions for Reviewers:

  • Click on the Percy Dashboard link to view detailed visual diffs.
  • Review the visual changes highlighted in the report.
  • Approve or request changes based on the visual differences.

@AlexVelezLl
Copy link
Member

Thanks @AadarshM07! It seems we found a bug in our KIcon fallback logic 😄. Could you include this little fix here too? We need to update this line to:

      if (this.selectedIcon.fixedColor && this.color) {

That means, updating the KolibriIcons[this.icon] with the computed property this.selectedIcon. This will fix this issue.

@AadarshM07
Copy link
Author

Hey @AlexVelezLl , Apologies, I think I forgot to pull the latest commits before pushing my changes. That might have caused the workflow to fail ?

@AadarshM07 AadarshM07 deployed to percy_tests June 1, 2025 13:50 — with GitHub Actions Active
@AadarshM07
Copy link
Author

I have resolved the failing lint workflow.

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Thanks @AadarshM07! LGTM!

@AlexVelezLl AlexVelezLl merged commit e29606d into learningequality:develop Jun 3, 2025
11 checks passed
learning-equality-bot bot pushed a commit that referenced this pull request Jun 3, 2025
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.

[Visual testing]: Add visual tests for KIcon
2 participants