-
Notifications
You must be signed in to change notification settings - Fork 126
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
fix(platform): icon-tab-bar _generateTabBarItems function improvement #12581
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for fundamental-ngx ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Visit the preview URL for this PR (updated for commit 9682b20): https://fundamental-ngx-gh--pr12581-12474-icon-tab-bar-o-mz63j9a8.web.app (expires Sun, 20 Oct 2024 12:34:00 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 41b993ee8e451bd7c6770b342ce142dc886eacff |
if (!('color' in item)) { | ||
item.color = 'default'; | ||
} | ||
item.color = item.color || 'default'; |
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.
What if you store item.color || 'default'
in a variable instead of modifying the value directly?
For example: const color = item.color || 'default'
You can then use this variable in cssClasses.
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.
Mutating item.color directly ensures that the item consistently has a valid color property in all cases inside return. By updating item.color early, it prevents potential future issues when item.color is referenced elsewhere in the result object or other parts of the code. Essentially, it is necessary for item.color to always have a valid value, but there is no better way to set it by default without similar mutation.
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.
LGTM
Related Issue(s)
closes #12474 #12554
Description
Screenshots
Before:
After:
Please check whether the PR fulfills the following requirements
During Implementation
PR Quality
https://github.com/SAP/fundamental-ngx/blob/main/CONTRIBUTING.md
https://github.com/SAP/fundamental-ngx/wiki/PR-Review-Checklist
README.md