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

Changed IconButton to KRouterLink #4374

Conversation

BabyElias
Copy link
Contributor

@BabyElias BabyElias commented Dec 29, 2023

Summary

Updated IconButton to use KComponents (KRouterLink, KIcon & KIconButton) instead.

Description of the change(s) you made

learningequality/kolibri-design-system#219

Screenshots (if applicable)

Screenshot-7

The icons that have been changed


Reviewer guidance

How can a reviewer test these changes?

This is just a trial PR to confirm that I am headed in the right direction or not.
If this pull request is accepted and merged, I plan to submit additional pull requests addressing other issue with more extensive changes. The current pull request contains only minor modifications related to that issue.


Contributor's Checklist

Testing:

  • Code is clean and well-commented
  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included

Reviewer's Checklist

This section is for reviewers to fill out.

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Just a couple of changes in the StagingTreePage/index.vue updates here.

Overall this is the exact right direction! Thanks so much @BabyElias for this submission!

@BabyElias
Copy link
Contributor Author

Thank you so much for your feedback!
Done with the required changes. If this PR is good to go, then I'll create separate PRs for changes at other places to ensure that checks are done for each change properly.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Thanks @BabyElias I've tested this locally and all looks good! Feel free to tag me for review on any follow-up issues you create along these lines.

@nucleogenesis nucleogenesis merged commit 13c143f into learningequality:unstable Jan 11, 2024
11 checks passed
@akolson akolson mentioned this pull request Aug 13, 2024
@akolson akolson mentioned this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants