-
Notifications
You must be signed in to change notification settings - Fork 162
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
refactor(icons): remove unnecessary icons and reuse the icon registration logic in Grid Filtering and QueryBuilder #14859
Open
teodosiah
wants to merge
21
commits into
master
Choose a base branch
from
thristodorova/feat-14604
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
2393573
refactor(icons): remove unnecessary icons & reuse the logic in Filter…
teodosiah daee624
fix(icons): resove circular dependency issue
teodosiah 4b38084
Merge branch 'master' into thristodorova/feat-14604
gedinakova bc6b908
Merge branch 'master' into thristodorova/feat-14604
teodosiah 7396911
Merge branch 'master' into thristodorova/feat-14604
ChronosSF 553434d
Merge branch 'master' into thristodorova/feat-14604
teodosiah 91f709d
Merge branch 'master' into thristodorova/feat-14604
teodosiah 2640343
feat(filtering): address comments and remove injection
teodosiah 489a16c
Merge branch 'master' into thristodorova/feat-14604
gedinakova 439617e
Merge branch 'master' into thristodorova/feat-14604
gedinakova aeb9257
Merge branch 'master' into thristodorova/feat-14604
teodosiah dede7ad
Merge branch 'master' into thristodorova/feat-14604
gedinakova bcb5a15
chore(*): revert filtering icons import
teodosiah 4ce6604
Merge branch 'master' into thristodorova/feat-14604
teodosiah c296eda
Merge branch 'thristodorova/feat-14604' of https://github.com/IgniteU…
teodosiah 4a2a718
Merge branch 'master' into thristodorova/feat-14604
teodosiah 7f814d5
Merge branch 'master' into thristodorova/feat-14604
teodosiah f8a1cf8
Merge branch 'master' into thristodorova/feat-14604
teodosiah 04f8fdf
Merge branch 'master' into thristodorova/feat-14604
teodosiah 4117e8a
Merge branch 'master' into thristodorova/feat-14604
teodosiah 57936b6
Merge branch 'master' into thristodorova/feat-14604
gedinakova File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import { InjectionToken, inject } from '@angular/core'; | ||
import { IMXIcon, contains, doesNotContain, endsWith, equals, greaterThan, greaterThanOrEqual, isAfter, isBefore, isEmpty, isFalse, isNull, isNotNull, isTrue, lastMonth, lastYear, lessThan, lessThanOrEqual, nextMonth, nextYear, notEmpty, notEqual, selectAll, startsWith, thisMonth, thisYear, today, ungroup, yesterday } from '@igniteui/material-icons-extended'; | ||
import { IgxIconService } from './icon.service'; | ||
|
||
export const filteringIcons: IMXIcon[] = [ | ||
contains, | ||
doesNotContain, | ||
endsWith, | ||
equals, | ||
greaterThan, | ||
greaterThanOrEqual, | ||
isAfter, | ||
isBefore, | ||
isEmpty, | ||
isFalse, | ||
isNotNull, | ||
isNull, | ||
isTrue, | ||
lastMonth, | ||
lastYear, | ||
lessThan, | ||
lessThanOrEqual, | ||
nextMonth, | ||
nextYear, | ||
notEmpty, | ||
notEqual, | ||
selectAll, | ||
startsWith, | ||
thisMonth, | ||
thisYear, | ||
today, | ||
ungroup, | ||
yesterday | ||
]; | ||
export const FILTERING_ICONS = new InjectionToken<void>('FILTERING_ICONS', { | ||
providedIn: 'root', | ||
factory: () => registerFilteringSVGIconsFactory(inject(IgxIconService)) | ||
}); | ||
|
||
export function registerFilteringSVGIconsFactory(iconService: IgxIconService) { | ||
filteringIcons.forEach(icon => { | ||
iconService.addSvgIconFromText(icon.name, icon.value, 'imx-icons'); | ||
}); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Okay, not exactly what I had in mind, but could consider.
First issue with the token approach is it doesn't tree-shake without explicit
/*@__PURE__*/
before it (see others in the repo) and thus drags the factory function and the icons collection with the exports from the package, thus making those permanently in the bundle. See the bundle app build warning here:https://github.com/IgniteUI/igniteui-angular/actions/runs/11162862017/job/31028455568#step:16:63
(just FYI same thing happens w/ just a chip component active).
The other issue with this is that due to this being injected (thus on component init) - these icons will be registered even when not in use, so again might be better as a simpler reusable function.
Also no reason for
filteringIcons
andregisterFilteringSVGIconsFactory
to be exported as far as I can tell.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.
There's also something going on with the extended icons - I get all of them bundled, which makes no sense and there might be an additional issue there as well.
Edit: Yeah, there seems to be an issue with the icons exports as they stand atm.
Even a simple
import { contains } from '@igniteui/material-icons-extended';
will pull the entire package it seems:That's with the Bundle test app that uses the default application builder. Possibly related to angular/angular-cli#26622, though not exactly as everything shakes out fine if the 'bundles' per category are not exported from the main barrel. Still digging.
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.
Okay, seems to be to do with the
all
export. If split like this:Tree-shaking seems to work fine