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

refactor(icons): remove unnecessary icons and reuse the icon registration logic in Grid Filtering and QueryBuilder #14859

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

teodosiah
Copy link
Contributor

Closes #14604

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@teodosiah teodosiah added squash-merge Merge PR with "Squash and Merge" option ❌ status: awaiting-test PRs awaiting manual verification and removed 🛠️ status: in-development Issues and PRs with active development on them labels Oct 1, 2024
ungroup,
yesterday
];
export const FILTERING_ICONS = new InjectionToken<void>('FILTERING_ICONS', {
Copy link
Member

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 and registerFilteringSVGIconsFactory to be exported as far as I can tell.

Copy link
Member

@damyanpetev damyanpetev Oct 7, 2024

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:
image
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.

Copy link
Member

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:

import {
    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/editor';

import { type IMXIcon } from '@igniteui/material-icons-extended';

Tree-shaking seems to work fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📈 enhancement filtering query-builder squash-merge Merge PR with "Squash and Merge" option ❌ status: awaiting-test PRs awaiting manual verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grid Filtering & Query builder redundant extended icons registration
4 participants