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.

Sign up for GitHub

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

Jump to bottom

Feature/angular 8 support #34

Merged
merged 3 commits into from
Nov 22, 2019
Merged

Feature/angular 8 support #34

merged 3 commits into from
Nov 22, 2019

Conversation

christophercr
Copy link
Collaborator

@christophercr christophercr commented Nov 15, 2019 •
edited
Loading

ISSUES CLOSED: #31

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[X] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #31

What is the new behavior?

  • ngx-form-errors officially supports Angular 8 now.
  • the demo is now implemented in the 2 latest versions of Angular: 7 and 8.
  • the installation, linting and unit testing of the demo apps for the 2 latest angular versions are included in the travis.yml file.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

@SuperITMan @nicanac you can see that the second commit of this PR adds two subfolders under /demo containing the demo app in the latest Angular version (8.x) and the previous one (7.x).

I suggest to keep these 2 versions of the demo app and maintain them with every new major version of Angular. For example, once Angular 9 is out, we keep the demo app in Angular 8 and 9, and so on.

Cause maybe with a new major version of Angular, it might not be possible to keep support for an old version. So at least we keep our demo with the 2 latest major versions so we are aligned with the Angular support schedule.

@christophercr christophercr added the enhancement New feature or request label Nov 15, 2019
@christophercr christophercr added this to the 1.0.0 milestone Nov 15, 2019
@christophercr
Copy link
Collaborator Author

christophercr commented Nov 15, 2019 •
edited
Loading

Well, it seems we need to change the node versions we use in Travis because the node 8 build of this PR is failing due to:

You are running version v8.16.2 of Node.js, which is not supported by Angular CLI 8.0+.
The official Node.js version that is supported is 10.9 or greater.

What do you guys think? Shall we change the node versions to 10 and 12?

@SuperITMan
Copy link
Member

The maintenance of Node 8 ends in January 2020 and in NBB we are now using Node 10 for our projects.
I think that we can drop the support of Node 8 for ngx-form-errors and for Stark and replace it by Node 12.
What do you think @christophercr @nicanac? 😊

@nicanac
Copy link
Contributor

nicanac commented Nov 15, 2019

The maintenance of Node 8 ends in January 2020 and in NBB we are now using Node 10 for our projects.
I think that we can drop the support of Node 8 for ngx-form-errors and for Stark and replace it by Node 12.
What do you think @christophercr @nicanac? 😊

yes, I fully agree. We don't need the version 8 anymore.

@christophercr
Copy link
Collaborator Author

Ok, so I'll replace node 8 by 12 😉

@christophercr
Copy link
Collaborator Author

Ok, node version changed and Travis builds passing! 👍

SuperITMan
SuperITMan requested changes Nov 18, 2019
View reviewed changes
Copy link
Member

@SuperITMan SuperITMan left a comment

Choose a reason for hiding this comment

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

Small changes

demo-app/ng8/src/app/pages/reactive-forms-example/reactive-forms-example.component.ts
this.formGroup = this.formBuilder.group({
username: [undefined, Validators.required],
matchingPasswords: new FormGroup(
{
Copy link
Member

@SuperITMan SuperITMan Nov 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The indentation is not the same in both demo-apps (ng7, ng8).
Could you please fix it to have "2 spaces" everywhere ? 😊

Copy link
Collaborator Author

@christophercr christophercr Nov 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I don't get it. What "2 spaces"?

I've ran Prettier on the whole project and the 2 demo-apps have exactly the same code formatting...

Copy link
Member

Choose a reason for hiding this comment

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

@christophercr my apologies...
I did the review with GitHub and I thought that the indentation was incorrect. But it's not. So please forget this comment 😊

demo-app/ng8/src/polyfills.ts Outdated
* APPLICATION IMPORTS
*/
// FixMe: Remove when NBB upgrades chrome browser (https://caniuse.com/#search=grid)
import "./polyfills/css-grid-layout-polyfill";
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove this import, and the file "css-grid-layout-polyfill", since we are using Chrome >v74 at NBB ? 😊
Or should we keep it for another reason?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah true, we should remove it. I'll remove it and test it also in IE 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've tested and indeed, we don't need the polyfill for Chrome, however it is necessary for IE but... the problem in IE with this polyfill is that it causes some flickering which is quite annoying.

So I think we should just remove the polyfill and let IE display the grid the best it can... which doesn't look too bad in my opinion, you know... it's IE 😕

@christophercr
Copy link
Collaborator Author

@SuperITMan @nicanac I've updated this PR with a couple of changes:

  • removed the CSS grid polyfill as suggested by @SuperITMan because it is no longer needed with the latest versions of Chrome (including the one used at the NBB).
  • disabled the host check from webpack-dev-server as mentioned here due to an error being logged constantly in IE console.

nicanac
nicanac approved these changes Nov 22, 2019
View reviewed changes
Copy link
Contributor

@nicanac nicanac left a comment

Choose a reason for hiding this comment

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

Perfect, thanks

SuperITMan
SuperITMan approved these changes Nov 22, 2019
View reviewed changes
Copy link
Member

@SuperITMan SuperITMan left a comment

Choose a reason for hiding this comment

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

LGTM ! 👍

@SuperITMan SuperITMan merged commit 0d19634 into NationalBankBelgium:master Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperITMan SuperITMan SuperITMan approved these changes

@nicanac nicanac nicanac approved these changes

Assignees
No one assigned
Labels
comp: demo enhancement New feature or request
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.

all: add support for Angular 8
3 participants
@christophercr @SuperITMan @nicanac