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

Use minified enterprise(-ng) versions in angular-soho app. #149

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

Conversation

swuendri
Copy link
Contributor

This PR is a replacement of #114 to use minified version of themes and cultures in applications generated with odin new. See also the issue #118. Thank you to @dohjon for the example!

Currently there is no option to include theme-new* files only.

<link rel="stylesheet" id="stylesheet" href="assets/ids-enterprise/css/theme-new-light.min.css" type="text/css">
<script>
// https://github.com/infor-design/enterprise/tree/main/src/components/locale#code-example---using-minified-files
window["SohoConfig"] = { minifyCultures: true };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to configure this in boilerplate/angular-soho/src/app/app.module.ts instead? In the APP_INITIALIZER where the locale is set.

Functionally there shouldn't be a difference, but it's nice to do the locale/culture config in a single place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The property Locale.minify of infor-design/enterprise project isn't "published" in https://github.com/infor-design/enterprise-ng/blob/main/projects/ids-enterprise-typings/lib/locale/soho-locale.d.ts to be directly used in Angular projects. I can add Soho.Locale['minify'] = true; to app.module.ts, but it looks a little bit tricky and isn't well "documented". How should we proceed @anhallbe? Should I create a PR in infor-design/enterprise-ng project to make the Locale.minify property available to Angular projects?

Copy link
Collaborator

@anhallbe anhallbe Nov 1, 2022

Choose a reason for hiding this comment

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

You could add the following to extend the SohoLocaleStatic typing, for better type safety and documentation:

// app.module.ts, or possibly polyfills.ts?

declare global {
   interface SohoLocaleStatic {
      /**
       * Instructs IDS to load minified localization files.
       * NOTE: Not part of the official IDS typings. Since this is not part
       * of the official API, it may stop working at any time.
       */
      minify: boolean;
   }
}

and then in the APP_INITIALIZER:

// app.module.ts

      {
         provide: APP_INITIALIZER,
         multi: true,
         deps: [LOCALE_ID],
         useFactory: (locale: string) => () => {
            Soho.Locale.minify = true;
            Soho.Locale.culturesPath = 'assets/ids-enterprise/js/cultures/';
            Soho.Locale.set(locale);
         },
      },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think @tmcconechy, should the project @infor-design/enterprise-ng publish the minify property from @infor-design/enterprise projects Locale? I would avoid unofficial features in odin new generated files. The official documentation in @infor-design/enterprice only mentions the following:

image

That's the reason why I'm not sure how to handle this situation.

@tmcconechy
Copy link

I think that setting just loads the node_modules/ids-enterprise/dist/js/cultures ones with .min.js instead of js.

So maybe you could use it if it still works? Or do the same and load the locales in with .min.js depending what your doing?

But they are published as minfied if you look in node_modules/ids-enterprise/dist/js/cultures

@swuendri
Copy link
Contributor Author

swuendri commented Nov 3, 2022

I believe my question wasn't precise enough. Let me try it again.

The Goal

We want to use the minified culture js files in a Angular project.

The documented way we can reach it

Setting the SohoConfig.minifyCultures to true before Soho.Locale will be initialized. We can reach this by putting the following code into index.html

<script>
      window["SohoConfig"] = { minifyCultures: true };
</script>

The way that we want to go

We want to set the usage of minified culture files in the same way we set the path to the cultures itself in our Angular module, before we set/load the current locale. Example:

            Soho.Locale.culturesPath = 'assets/ids-enterprise/js/cultures/';
            Soho.Locale.minifyCultures = true;

Why we can't/won't go this way

Currently there is no "public" property in Soho.Locale to force the loading of minified culture files.
But there is a property called minify in class Locale of ids-enterprise, which is initialized by the value of SohoConfig.minifyCultures. And this property is also mentioned as parameter to initialize the Locale class.
image
On the other hand Locale.minify isn't named in ids-enterprise-ng typings SohoLocaleStatic (https://github.com/infor-design/enterprise-ng/blob/main/projects/ids-enterprise-typings/lib/locale/soho-locale.d.ts). So we cannot easily use the property.

The final question

Is it possible to introduce the property minify in ids-enterprise-ng typings @tmcconechy? Or is the property more of a private one?

@swuendri
Copy link
Contributor Author

@anhallbe, do not merge the PR now. The build of Angular Soho App breaks as expected but the workflow is successful. I will take a look on it.

@swuendri
Copy link
Contributor Author

@anhallbe, I have changed the cli/src/cli.ts file to return an error in case of unsuccessful build. Should I create a separate PR for this? Or is it okay to change it within this PR?

No the build breaks, which is my expected behavior for the moment. If there is a newer version of enterprise-ng available with our change, I trigger here again.

@anhallbe
Copy link
Collaborator

You can keep it in the PR

@swuendri
Copy link
Contributor Author

@anhallbe, ids-enterprise-ng contains our necessary change starting with version 14.7.0. There is currently the first bug fix version available which I introduced here. In my opinion this PR is ready to merge.

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.

3 participants