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

Angular4 upgrade #118

Merged
merged 12 commits into from Apr 15, 2017
Merged

Angular4 upgrade #118

merged 12 commits into from Apr 15, 2017

Conversation

JulienMrgrd
Copy link
Member

This PR contains the Registry upgrade, from Angular 2 to Angular 4.
It contains also some code formatting (#113) and CSS changes.

A part of this work has been based on a generated Gateway, with Angular 4, in v4.2.0.

N.B : Most of changes here concerns the front, so we have to be careful with all next PR, concerning Angular 2 (#98 #99 #112 #114 #116).
N.B.2 : In history.component.html, do not pay attention to the <template> element for the moment. The Ng-Bootstrap team works on this (ng-bootstrap/ng-bootstrap#1337).

All visuals

capture d ecran de 2017-04-14 16-11-04
capture d ecran de 2017-04-14 16-11-09
capture d ecran de 2017-04-14 16-11-12
capture d ecran de 2017-04-14 16-11-17
capture d ecran de 2017-04-14 16-11-22
capture d ecran de 2017-04-14 16-11-25
capture d ecran de 2017-04-14 16-11-28
capture d ecran de 2017-04-14 16-11-35
capture d ecran de 2017-04-14 16-11-42
capture d ecran de 2017-04-14 16-11-50
capture d ecran de 2017-04-14 16-11-58
capture d ecran de 2017-04-14 16-12-04

@deepu105
Copy link
Member

deepu105 commented Apr 14, 2017 via email

Copy link
Member

@deepu105 deepu105 left a comment

Choose a reason for hiding this comment

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

Overall looks good. I have some comments about re usability.

"clientPackageManager": "yarn"
}
}
"generator-jhipster": {
Copy link
Member

Choose a reason for hiding this comment

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

why is indentation changed?

"jwtSecretKey": "c9d37cefc48581919939d587c750ea215020765b",
"useSass": false,
"enableTranslation": false,
"applicationType": "microservice",
Copy link
Member

Choose a reason for hiding this comment

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

I think making application type to gateway would be more appropriate

@@ -6,7 +6,7 @@
<artifactId>spring-boot-starter-parent</artifactId>
<groupId>org.springframework.boot</groupId>
<version>1.5.2.RELEASE</version>
<relativePath/>
<relativePath />
Copy link
Member

Choose a reason for hiding this comment

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

why space?

@NgModule({
imports: [
JhipsterRegistrySharedModule,
RouterModule.forRoot(adminState, {useHash: true})
RouterModule.forRoot(adminState, { useHash: true }),
NgbModule.forRoot(),
Copy link
Member

Choose a reason for hiding this comment

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

why this import, its already imported by JhipsterRegistrySharedModule and hence should be available here?

<h4 class="modal-title" id="showHealthLabel"><span class="text-capitalize">{{ baseName(currentHealth.name) }}</span>
{{subSystemName(currentHealth.name)}}
</h4>
{{subSystemName(currentHealth.name)}}</span></h4>
Copy link
Member

Choose a reason for hiding this comment

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

format this part properly so that span and h4 are in new lines

@@ -11,12 +11,22 @@ Metrics page styles
text-align: initial;
}

span.label { /* UP, DOWN, ... */
/* ==========================================================================
Routes styles
Copy link
Member

Choose a reason for hiding this comment

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

why is this called route styles?
Please move this into the main app css and make it class and apply it everywhere instead of duplicating this everywhere

Copy link
Member

Choose a reason for hiding this comment

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

do the same for all common styles

@@ -10,7 +10,7 @@
<ul class="list-group">
Copy link
Member

Choose a reason for hiding this comment

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

this code seems to be used in many components. It should be made into a re usable component and then included everywhere

if (index !== -1) {
this.routes[index].status = 'DOWN';
}
}
}

// user click
getLabelClassRoute(route: Route) {
getBadgeClassRoute(route: Route) {
Copy link
Member

Choose a reason for hiding this comment

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

same for the duplicated methods. these should go into the reusable componenet

@@ -33,6 +33,14 @@ <h4 class="modal-title">Sign in</h4>
</div>
<button type="submit" class="btn btn-primary">Sign in</button>
</form>
<p></p>
<div class="alert alert-warning">
Copy link
Member

Choose a reason for hiding this comment

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

Are these required? as we dont have a user management for registry

@deepu105
Copy link
Member

I'm working on this. I'll merge it manually

@JulienMrgrd @mohamed2a @jhipster/registry-developers please do not change generated code unless there is a need for it as it will make upgrades harder. If you find an issue in the generated code fix it at root (In the generator)
What was the reason t rewrite integration tests with Mockito?

@deepu105
Copy link
Member

@jdubois could you please wait for my review for new PR's?

@jdubois
Copy link
Member

jdubois commented Apr 15, 2017

Yes of course

@deepu105 deepu105 merged commit 7a06d5a into jhipster:develop Apr 15, 2017
@deepu105
Copy link
Member

I have refactored the front end code a bit. I have pushed it but its WIP. I'll be pushing more tomorrow

@deepu105
Copy link
Member

Its done.
@jhipster/registry-developers please note that I have refactored the front end code a lot. I have made as much as possible to align with generated code so that we can easily upgrade when we release new versions. Also I have switched to SASS for css
@JulienMrgrd @mohamed2a @g-boy05 Please do not change generated code unless absolutely necessary. If there is any error in generated code fix it in the jhipster-generator first with a PR.
let me know if you have any concerns.

@JulienMrgrd
Copy link
Member Author

Really nice refactoring @deepu105 ! (especially the menus and the "route" component that I made)

OK concerning the generated code ;)

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.

None yet

3 participants