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

Tabs refactor #257

Merged
merged 22 commits into from
Feb 1, 2021
Merged

Tabs refactor #257

merged 22 commits into from
Feb 1, 2021

Conversation

clemiller
Copy link
Contributor

@clemiller clemiller commented Dec 30, 2020

Refactor of the tab/tabs component to resolve the performance issues caused by opening multiple tabs.

  • Removal of the dynamic tabs directive
  • A single data-table is used to display all of the layers; opening a "new" tab will hide the data-table
  • Moved the help page and SVG exporter to a pop-up dialog window

Addresses issue #254

@clemiller clemiller requested a review from isaisabel December 30, 2020 15:48
Copy link
Contributor

@isaisabel isaisabel left a comment

Choose a reason for hiding this comment

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

Your imports here are based on an outdated version of material from before the packages were updated. You should npm install to get the latest packages.
The fix is that the import of dialog stuff in help.component.ts and exporter.component.ts:

import { MatDialogRef, MAT_DIALOG_DATA } from '@angular/material';

should be:

import { MatDialogRef, MAT_DIALOG_DATA } from '@angular/material/dialog';

Even despite this fix, I'm still having issues:

ng serve
? Port 4200 is already in use.
Would you like to use a different port? Yes
✔ Browser application bundle generation complete.

Initial Chunk Files | Names         |      Size
scripts.js          | scripts       |   2.70 MB
vendor.js           | vendor        | 340.34 kB
styles.css          | styles        |  74.76 kB
runtime.js          | runtime       |   6.15 kB
main.js             | main          |   1.80 kB
polyfills.js        | polyfills     | 683 bytes

                    | Initial Total |   3.11 MB

Build at: 2021-01-04T15:54:29.794Z - Hash: ebdc5e1d8a18ab77f0e5 - Time: 11649ms

Warning: Zone.js does not support native async/await in ES2017+.
These blocks are not intercepted by zone.js and will not triggering change detection.
See: https://github.com/angular/zone.js/pull/1140 for more information.



Error: src/app/tab/tab.component.ts:19:17 - error NG2003: No suitable injection token for parameter 'title' of class 'TabComponent'.
  Consider using the @Inject decorator to specify an injection token.

19     constructor(title: string, isCloseable: boolean, showScoreVariables: boolean, domain: string, dataTable: boolean) {
                   ~~~~~

  src/app/tab/tab.component.ts:19:24
    19     constructor(title: string, isCloseable: boolean, showScoreVariables: boolean, domain: string, dataTable: boolean) {
                              ~~~~~~
    This type is not supported as injection token.
src/app/app.module.ts:43:5 - error NG6001: The class 'TabComponent' is listed in the declarations of the NgModule 'AppModule', but is not a directive, a component, or a pipe. Either remove it from the NgModule's declarations, or add an appropriate Angular decorator.

43     TabComponent,
       ~~~~~~~~~~~~

  src/app/tab/tab.component.ts:8:14
    8 export class TabComponent {
                   ~~~~~~~~~~~~
    'TabComponent' is declared here.




** Angular Live Development Server is listening on localhost:55928, open your browser on http://localhost:55928/ **

Since I can't get past these errors, I can't actually review the UX/UI much at all.

One thing I am assuming from my skimming of the HTML changes is that there is no explicit close button for the help and exporter dialogs. While the user can simply click off the dialogs to close them, they might not know this and think they're stuck in those UIs. Adding a close button at the top right or top left of the dialogs will alleviate this. Ideally the close button won't scroll with the dialog content so the user can see it wherever they are in the help page for instance.

I'll do a more comprehensive review once the build issues are fixed.

nav-app/src/app/globals.ts Show resolved Hide resolved
nav-app/src/app/help/help.component.html Outdated Show resolved Hide resolved
Copy link
Contributor

@isaisabel isaisabel left a comment

Choose a reason for hiding this comment

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

Overall most of this looks pretty good. However, the SVG exporter really needs some polish. Currently it looks like this:
image

  1. There should be no padding around the toolbar at the top, or the entire SVG area should have a grey-outlined box around it like the main datatable does. Currently it looks very strange that the toolbar doesn't extend to the full width of the dialog. Adding the border would also alleviate this issue I think.
  2. The close button has some sort of bug when the dialog opens. I believe it is focused when the dialog opens, so it has a grey background until you click other elements of the dialog. It also has a lot of whitespace around it because the dialog has built-in margins on the bottom. Removing the bottom margin from the button's div will make it visually nicer (and vertically centered in the whitespace).

Before (with extra margin):
Screen Shot 2021-01-11 at 9 57 15 AM
After (without extra margin):
Screen Shot 2021-01-11 at 9 57 22 AM

I'm sure you're aware of this, but #251 is still a problem (just switching quickly between two layer tabs eventually causes the freeze, I think that's the easiest way to reproduce it), and using the relative links in the help dialog closes the dialog (issue documented here without a solution).

nav-app/src/app/tabs/tabs.component.ts Show resolved Hide resolved
@clemiller clemiller marked this pull request as ready for review January 14, 2021 14:44
@clemiller clemiller changed the title WIP: tabs refactor Tabs refactor Jan 14, 2021
@clemiller clemiller requested a review from isaisabel January 14, 2021 14:45
Copy link
Contributor

@isaisabel isaisabel left a comment

Choose a reason for hiding this comment

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

Looks great, though #263 needs to merge into this branch before we merge this one to develop.

…tions

Additional performance improvements for tabs and data subscriptions
@isaisabel isaisabel merged commit 6915e73 into develop Feb 1, 2021
@clemiller clemiller deleted the bugs/tabs-refactor branch February 17, 2022 16:45
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.

2 participants