-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[Message Icons] Add special character screening for 'marked' icons #4110
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
Conversation
…ltering at the end for matching
Added warning about modifying Lib.js file to README.
Screen for special characters at the end of an app name and remove them for better matching.
Clarify instructions regarding icon generation and character removal.
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.
Pull request overview
This PR enhances the message icons library to handle special characters in app names (e.g., Slack* → Slack) and addresses missing color definitions from a previous PR. The changes improve icon matching compatibility when apps are marked with special characters.
Key Changes:
- Adds regex-based special character screening at the end of app names (
*!?.-_and whitespaces) - Backfills missing color definitions for recently added icons (adp, duolingo, roborock, shortcuts)
- Improves documentation and warnings to prevent future mistakes with manual lib.js modifications
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/messageicons/metadata.json | Version bump from 0.13 to 0.14 |
| apps/messageicons/lib.js | Adds special character trimming logic and alphabetizes color definitions |
| apps/messageicons/icons/generate.js | Updates generator to produce the new special character trimming code and includes missing color definitions |
| apps/messageicons/README.md | Adds documentation about special character handling and strengthens warnings about not modifying lib.js |
| apps/messageicons/ChangeLog | Documents the new special character screening feature in version 0.14 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…/BangleApps into messageIconsSpecChar-fix
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.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bobrippling
left a comment
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.
Shall let @gfwilliams do the main review as it's a core app, but a few things I noticed :)
Co-authored-by: Rob Pilling <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
Thanks, although IMO I don't think we should be trying to filter out special chars. For instance by filtering out ... given the only reason for this is for filtering out the If you wanted to strip out that code and keep the colour changes that'd be good though. |
|
Could we only strip end characters that are special instead? |
|
I just think it's a slippery slope - the messageicons lib should really just take app name (ideally more of an ID) and return an icon, not try and perform other functions. I don't really get why you wanted the |
Removed special character trimming from message string.
|
Ok, just updated this to only change the icon color bug fixes, and added a new health icon. I removed the character filtering. I think this can be merged, although I still want to do some testing to #4108 before we merge that too. |
|
Thanks! Looks great! |
This just increases match compatibility even if an app sets the src to be 'Slack*'. This is made to fix the issue at PR #4108
Also updated the generate.js file, as I only added colors for the previous icons I added to the lib.js in #4105
I also added more documentation and warning to help others not make this mistake