Skip to content

[Icons] Add AppRegistrationIcon component#1492

Merged
Rajesh-Nagarajan-11 merged 3 commits intolayer5io:masterfrom
aprv10:feat/app-registration-icon
May 8, 2026
Merged

[Icons] Add AppRegistrationIcon component#1492
Rajesh-Nagarajan-11 merged 3 commits intolayer5io:masterfrom
aprv10:feat/app-registration-icon

Conversation

@aprv10
Copy link
Copy Markdown
Contributor

@aprv10 aprv10 commented May 6, 2026

Notes for Reviewers

This PR fixes #1487

Signed commits

  • Yes, I signed my commits.

Signed-off-by: aprv10 <1apoorvindia@gmail.com>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the AppRegistrationIcon component and its local export file. Feedback was provided to improve consistency by using the DEFAULT_FILL_NONE constant and to fix a potential bug where spreading props could overwrite the fill attribute with an undefined value. Additionally, the reviewer noted that the new icon should be exported from the central icons entry point.

Comment thread src/icons/AppRegistration/AppRegistrationIcon.tsx Outdated
Comment thread src/icons/AppRegistration/index.ts Outdated
Signed-off-by: aprv10 <1apoorvindia@gmail.com>
@suzy-g38
Copy link
Copy Markdown

suzy-g38 commented May 7, 2026

@aprv10 can you provide a recording with the dummy implementation of app-registration-icon at meshery-ui? I think it will be helpful to visualise

@aprv10
Copy link
Copy Markdown
Contributor Author

aprv10 commented May 7, 2026

@suzy-g38 I’ve been trying to get the UI working for quite some time now. I’ve set up the codebase locally but I still don’t see the icon reflected in the meshery UI.

If possible, could you guide me on how I can properly test the AppRegistrationIcon implementation locally? That would be really helpful. From my side the implementation seems correct so I think I might be missing something in the setup or integration step.
Attaching image for reference:
Screenshot 2026-05-08 032428

Comment thread src/icons/index.ts
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@aprv10, so many changes have been made to this file. Please revert and only add the relevant changes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Most of the changes are from the linting fixes applied during the build process. The lines were mainly reformatted/moved around, so there’s effectively no functional change in that file apart from the export I added.

I’ll keep this in mind going forward and avoid including unrelated formatting changes.

Copy link
Copy Markdown
Member

@Rajesh-Nagarajan-11 Rajesh-Nagarajan-11 left a comment

Choose a reason for hiding this comment

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

LGTM , Cross verified path with mui-icon

@Rajesh-Nagarajan-11 Rajesh-Nagarajan-11 merged commit 2e0acde into layer5io:master May 8, 2026
2 checks passed
@aprv10 aprv10 deleted the feat/app-registration-icon branch May 8, 2026 17:26
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.

[Feature] Add AppRegistrationIcon to Sistent

4 participants