ISO-TC211 / iso-geodetic-registry

ISO Geodetic Registry backend
https://registry.isotc211.org
Other
5 stars 4 forks source link

Home page #66

Closed bbehling closed 4 years ago

bbehling commented 4 years ago

Hi @bbehling, please don't check in files/folders like .DS_Store but add them to .gitignore instead.

Hi @florianesser. I removed the .DS_Store files and added them to the 'second' gitignore file. We have two .gitignore files. One at the root, one in src. Should there be only one?

florianesser commented 4 years ago

We have two .gitignore files. One at the root, one in src. Should there be only one?

It's fine to have additional .gitignore files in subdirectories, e.g. if you need specific ignores in one directory but not in another.

phuonghuynh commented 4 years ago

Hi @bbehling Is this PR included left-tree in the homepage? Also, could you rename class HelloWorld.java to something, might be HomePageController and remove all test code.

I think using Bootstrap SCSS is better than the "CSS" version, since SCSS compiler will compile only used Bootstrap CSS Classes in the application. We might don't need all bootstrap classes.

ronaldtse commented 4 years ago

I think using Bootstrap SCSS is better than the "CSS" version, since SCSS compiler will compile only used Bootstrap CSS Classes in the application. We might don't need all bootstrap classes.

Agree with @phuonghuynh !

bbehling commented 4 years ago

Hi @bbehling Is this PR included left-tree in the homepage? Also, could you rename class HelloWorld.java to something, might be HomePageController and remove all test code.

I think using Bootstrap SCSS is better than the "CSS" version, since SCSS compiler will compile only used Bootstrap CSS Classes in the application. We might don't need all bootstrap classes.

Ok. I'll make those changes. I did not include the tree view yet.

bbehling commented 4 years ago

Not sure if this has to be pixel perfect to the legacy UI, so I used bootstrap defaults.

phuonghuynh commented 4 years ago

Thanks @bbehling

Do you think we can replace Font Awesome CSS by SCSS version also?

I think it is better to implement the tree-view in this PR. We dont' need to handle clicked-on-node action now, we can display tree-view + data for it only.

Also, I am not sure if we need "ng-boostrap", since we can mostly handle UI Elements with Bootstrap + Angular. I think if we don't need to use it, we can remove it for now. We can also add it later.

For example: NgBoostrap version for button look like this

  <label class="btn-primary" ngbButtonLabel>
    <input type="checkbox" ngbButton [(ngModel)]="model.right"> Right
  </label>

Poor Angular+Boostrap like this:

 <label class="btn-primary">
    <input type="checkbox" [(ngModel)]="model.right" (ngModelChange)="onChanged($event)"> Right
  </label>
bbehling commented 4 years ago

Thanks @bbehling

Do you think we can replace Font Awesome CSS by SCSS version also?

I think it is better to implement the tree-view in this PR. We dont' need to handle clicked-on-node action now, we can display tree-view + data for it only.

Also, I am not sure if we need "ng-boostrap", since we can mostly handle UI Elements with Bootstrap + Angular. I think if we don't need to use it, we can remove it for now. We can also add it later.

For example: NgBoostrap version for button look like this

  <label class="btn-primary" ngbButtonLabel>
    <input type="checkbox" ngbButton [(ngModel)]="model.right"> Right
  </label>

Poor Angular+Boostrap like this:

 <label class="btn-primary">
    <input type="checkbox" [(ngModel)]="model.right" (ngModelChange)="onChanged($event)"> Right
  </label>

HI @phuonghuynh ng bootstrap gives us the ability to easily tree shake, and only include the components we want. I know it's possible to do this with regular bootstrap. However with ng bootstrap, we don't have to configure anything in webpack.

Tree shaking saves us 3mb versus bringing in the entire js library.

phuonghuynh commented 4 years ago

Tree shaking saves us 3mb versus bringing in the entire js library.

Yep, just in case if we use some modules in bootstrap components, like popover (tooltips or something). If we only need Bootstrap CSS, we don't need to add this module. However, it is OK if you still want to have it.

phuonghuynh commented 4 years ago

Hi @bbehling,

I got error about CROS when testing, could you help to implement corporate-proxy to allow Angular can work with the API?

Or we can add Spring CORS support, https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/cors/CorsConfiguration.html

Access to XMLHttpRequest at 'http://localhost:8080/findAll' from origin 'http://localhost:4200' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

We can do hardcode for API endpoint in proxy-config now, https://localhost:8080/*, then we can have new profile for production.

bbehling commented 4 years ago

Hi @bbehling,

I got error about CROS when testing, could you help to implement corporate-proxy to allow Angular can work with the API?

Or we can add Spring CORS support, https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/cors/CorsConfiguration.html

Access to XMLHttpRequest at 'http://localhost:8080/findAll' from origin 'http://localhost:4200' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

We can do hardcode for API endpoint in proxy-config now, https://localhost:8080/*, then we can have new profile for production.

HI @phuonghuynh

I added @CrossOrigin annotations to the API end points which resolved the CORS issues in my dev env. This should be checked in with this PR. Does that work in your environment?

phuonghuynh commented 4 years ago

Not working on my side, we also need to support cors for our origins only. We don't want to allow all outside calls

bbehling commented 4 years ago

Check comment for more details

This looks like a simple option https://angular.io/guide/build#proxying-to-a-backend-server.

I added that and removed the @CrossOrigin attribute from the REST api. Thats working good on my end.

If you have a corporate proxy setup, let me know and will implement it in Angular.

phuonghuynh commented 4 years ago

@bbehling its working. Also could you add some loading box since Lambda loading is too slow, it need to resolve too much dependencies using in the application. I think we can not fix this since we are using Spring Framework except we rework on the DB-layer and use Dagger/Guice for DI, ref-link: https://docs.aws.amazon.com/lambda/latest/dg/best-practices.html

Check this record link for local-lambda running and you can see the loading is too slow.

One more note is that we should call one rest for one page to fetch all data we needed for that page? I not sure, but Lambda will run new context for each comming request, ref-link https://docs.aws.amazon.com/lambda/latest/dg/runtimes-context.html

For example, in this PR, there are 2 calls:

Lambda-local report:

END RequestId: 34d11d3c-a82a-1562-9393-08985c19365e
REPORT RequestId: 34d11d3c-a82a-1562-9393-08985c19365e  Init Duration: 1387.16 ms       Duration: 41699.79 ms   Billed Duration: 41700 ms       Memory Size: 1512 MB Max Memory Used: 503 MB 

END RequestId: cfd87daf-6438-1300-4578-ed15c07b2719
REPORT RequestId: cfd87daf-6438-1300-4578-ed15c07b2719  Init Duration: 1472.86 ms       Duration: 42987.21 ms   Billed Duration: 43000 ms       Memory Size: 1512 MB Max Memory Used: 516 MB 

IMO, static resources, such as "version_string", "images" can be in the "front-end" code.

/cc @ronaldtse

bbehling commented 4 years ago

@bbehling its working. Also could you add some loading box since Lambda loading is too slow, it need to resolve too much dependencies using in the application. I think we can not fix this since we are using Spring Framework except we rework on the DB-layer and use Dagger/Guice for DI, ref-link: https://docs.aws.amazon.com/lambda/latest/dg/best-practices.html

Check this record link for local-lambda running and you can see the loading is too slow.

One more note is that we should call one rest for one page to fetch all data we needed for that page? I not sure, but Lambda will run new context for each comming request, ref-link https://docs.aws.amazon.com/lambda/latest/dg/runtimes-context.html

For example, in this PR, there are 2 calls:

  • Get Static version string
  • Get data for the treeview

Lambda-local report:

END RequestId: 34d11d3c-a82a-1562-9393-08985c19365e
REPORT RequestId: 34d11d3c-a82a-1562-9393-08985c19365e  Init Duration: 1387.16 ms       Duration: 41699.79 ms   Billed Duration: 41700 ms       Memory Size: 1512 MB Max Memory Used: 503 MB 

END RequestId: cfd87daf-6438-1300-4578-ed15c07b2719
REPORT RequestId: cfd87daf-6438-1300-4578-ed15c07b2719  Init Duration: 1472.86 ms       Duration: 42987.21 ms   Billed Duration: 43000 ms       Memory Size: 1512 MB Max Memory Used: 516 MB 

IMO, static resources, such as "version_string", "images" can be in the "front-end" code.

/cc @ronaldtse

Added loading Icon and HTTP interceptors.

Can me we merge this request and move any additional items to a new PR?

The version is part of the footer, not the home page. We are going to make separate calls when clicking the tree view to get data for the table, and additional calls for pagination.

ronaldtse commented 4 years ago

Thanks for merging this. Images etc static assets, and the compiled front end, should al rely on cloudfront instead.