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

ng-jhipster migration to generator-jhipster #13022

Merged
merged 184 commits into from
Nov 16, 2020

Conversation

kaidohallik
Copy link
Contributor

This PR removes ng-jhipster dependency from generator-jhipster.

Closes #12909

This PR contains commits from https://github.com/jhipster/ng-jhipster extracted by method described in #12909 (comment)

Probably not all ng-jhipster committers from period 2016-2020 are signed CLA, so probably needs to be merged without passing CLA check.

As we have strict TypeScript option and TypeScript ESLint "plugin:@typescript-eslint/recommended-requiring-type-checking" then I was forced to do quite a lot changes to get migrated functionality to work in generator-jhipster.

Some comments about changes by commits.

sort

In the process of migration fixed #12868

metrics

  • split code to .ts and .html as Angular style guide suggests
  • declared also model, so fixed one "@typescript-eslint/no-explicit-any" as referenced also in Strict Typescript option in metrics module in Angular #10873 (comment)
  • used ChangeDetectionStrategy.OnPush where functions was used in html - performance improved a bit
  • improved filter section in threads modal window - instead of showing filter in text input, badges are now directly reflecting current filtering state
  • temporarily added -new to component selectors to avoid conflicts with ng-jhipster, those are removed by the ng-jhipster removal commit

event-manager

In jhipster/ng-jhipster#115 I made preparation to simplify output to event manager consumers - return only event.content because event name is redundant, subscription is by name. But now we have #12778 - multiple subscriptions, it's a good idea and in this case returning full info, including event name, is must have. So I deleted this simplification preparation and we should go #12778 way.

jhi-boolean

As this is used only in one place, user management detail view (and if skipping user management, for example oauth2, then there is 0 usages), and if this component is not widely used and it's API learned then code without this component is better understandable, so I didn't migrate this component but instead using simple html in user management detail view (lines used in .ejs template reduced from 10 to 2).

jhi-item-count

Temporarily added -new to component selector to avoid conflicts with ng-jhipster, this is removed by the ng-jhipster removal commit.

As in current implementation:

  • HTML was hacked and very ugly to put somehow i18n disabled and enabled to work together in one component
  • perfomance was bad, every event in page caused recalculation of jhi-item-count (for example every click resulted with 6 time recalculation)
  • calculation was implemented twice, one for i18n enabled and other for i18n disabled
  • if i18n disabled then calculation was not unit tested

Then rewrote component, now:

  • HTML is now very minimalistic 1-liner (achieved by using enableTranslation variable available in generator)
  • item-count component is recalculated only if input changes (different page is selected)
  • common calculation for both i18n disabled and enabled
  • as there is common solution for i18n disabled and enabled then both are now also unit tested

data-util

  • removed unused functionality and tests for unused functionality (a lot was removed)
  • openFile function has 2 forks, comment was that the first one is for IE and Edge, but for Edge that was not true, so changed comment from "To support IE and Edge" to "To support IE"

parse-links

Polished and made shorter

alert.service

Now when we have full control over this component (we can use powerful if (enableTranslation) { ... }) we should refactor this service to offer better API to avoid hacky workarounds as described in closing #12796. But this enhancement is out of scope of this PR.

translate

  • in this commit removed also ng-jhipster dependency
  • now we don't need to force @ngx-translate dependency for i18n disabled any more
  • after removing ng-jhipster dependency, core.module is quite small and after merging this PR I want to move code from core.module to app.module and delete core.module to reduce code hierarchy level

Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

deepu105 and others added 30 commits December 26, 2016 17:07
Added service revamp and tests
use const spread declarations
make ng2-translate a dependency
Prevent error when broadcasting and there's no subscriber
Display right translation key when missing
I wrote a unit test that shows that an alert is not removed from alerts
array on timeout.
I guess there is an ambiguity about extAlerts, I don't understand its
goal because when adding a new alert added to extAlerts and so it cannot
be removed on close.
migration notes:
* split code to .ts and .html as Angular style guide suggests
* declared also model, so fixed one no-explicit-any as referenced also in jhipster#10873 (comment)
* used ChangeDetectionStrategy.OnPush where functions was used in html - performance improved a bit
* improved threads modal window filter section - instead of showing filter in text input, badges are now directly reflecting current filtering state
@CLAassistant
Copy link

CLAassistant commented Nov 11, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
8 out of 9 committers have signed the CLA.

✅ deepu105
✅ wmarques
✅ jdubois
✅ vishal423
✅ kaidohallik
✅ SudharakaP
✅ mshima
✅ pascalgrimaud
❌ Vivek More


Vivek More seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@pascalgrimaud
Copy link
Member

really good job @kaidohallik !
I'm trusting you here, and unless someone wants to review it, I'll probably merge it really soon

@mshima
Copy link
Member

mshima commented Nov 11, 2020

I have tested yesterday, and everything looks fine, I forgot to test sorting.
One thing I've noticed it that the UI does not change to the account language once logged in, but I don't know if it's a regression.

Bundle at our ngx-default changed from:

chunk {2} main.2c37efdf00f81a130127.css, main.9268b221dd8c8f2aad31.js (main) 853 kB [initial] [rendered]
64

to

chunk {2} main.2c37efdf00f81a130127.css, main.83dd4bcfd4386bf44784.js (main) 801 kB [initial] [rendered]
63

IMO let's merge, and finish testing it in main branch.

@kaidohallik
Copy link
Contributor Author

One thing I've noticed it that the UI does not change to the account language once logged in, but I don't know if it's a regression.

I tested this and this was working for me. You probably changed language before logging in? Logic in application is: if user has changed language then in this session don't change language on login any more.

@mshima mshima merged commit 6ec3473 into jhipster:main Nov 16, 2020
@kaidohallik
Copy link
Contributor Author

Thanks @mshima for merging!

@kaidohallik
Copy link
Contributor Author

@pascalgrimaud
Copy link
Member

@kaidohallik : approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ng-jhipster Navigating back-forward in paginated entity sort icon doesn't change in Angular