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

DSP-869 add select project component #213

Merged
merged 15 commits into from
Nov 24, 2020
Merged

Conversation

waychal
Copy link
Contributor

@waychal waychal commented Oct 28, 2020

resolves DSP-869

It adds the select project component with autocompletion feature.

In component, filtered project list for given user IRI is provided to select a project. When project is selected, it emits the event to the parent and returns the project's short name.

Snehal Kumbhar added 3 commits October 28, 2020 11:59
- New component in ui-lib (action module) to select project which can be used in fulltext,
  but also in advanced search (DSP-292)
- Add filter to search project by short name, long name or short code
- Send project info back to parent
- Add input parameter for user IRI to only display projects that belkongs to this user
@waychal waychal added the enhancement New feature or request label Oct 28, 2020
@waychal waychal self-assigned this Oct 28, 2020
Comment on lines 2 to 5
.dsp-select-project-component {
&.select-form {
margin-top: -18px;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This part is not needed. The defined style has no effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 16 to 19
.select-project-btn {
margin: 0 0 16px;
min-width: 140px;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This part is not needed. The defined style has no effect when the class is not used in the template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 1 to 16
import { OverlayModule } from '@angular/cdk/overlay';
import { HarnessLoader } from '@angular/cdk/testing';
import { TestbedHarnessEnvironment } from '@angular/cdk/testing/testbed';
import { Component, ViewChild } from '@angular/core';
import { async, ComponentFixture, fakeAsync, TestBed } from '@angular/core/testing';
import { FormsModule, ReactiveFormsModule } from '@angular/forms';
import { MatAutocompleteModule } from '@angular/material/autocomplete';
import { MatAutocompleteHarness } from '@angular/material/autocomplete/testing';
import { MatOptionModule } from '@angular/material/core';
import { MatIconModule } from '@angular/material/icon';
import { MatInputModule } from '@angular/material/input';
import { MatSelectModule } from '@angular/material/select';
import { BrowserAnimationsModule } from '@angular/platform-browser/animations';
import { MockProjects, ProjectsEndpointAdmin, UsersEndpointAdmin } from '@dasch-swiss/dsp-js';
import { of } from 'rxjs/internal/observable/of';
import { DspApiConnectionToken } from '../../../core';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please clean up the the imports by running "Organize imports" command in your IDE.
https://docs.google.com/document/d/1SIVMxoV0N8xYqhVk_0hhuqSuUF47D1YYrDESqdmipfM/edit#heading=h.dayyw3sk2234

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 16 to 34
// list of projects
projects: ReadProject[];

// selected project
selectedProject: ReadProject;

// filter projects while typing (autocomplete)
filteredProjects: Observable<ReadProject[]>;

// form group
selectProjectForm: FormGroup;

// in case of an (api) error
error: any;

// user IRI so we could show only projects that belongs to this user
@Input() userIri?: string;

@Output() projectInfo: EventEmitter<string> = new EventEmitter<string>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow our Coding Conventions about "Variable declaration order"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

projects: ReadProject[];

// selected project
selectedProject: ReadProject;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the @output().

I will add a comment how to do it...

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can find selected project in list of projects by looking for shortcode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the code to return ReadProject object. But currently I am looping the complete projects array to get the selected project info. It would be good to have dictionary and not array for all projects to improve performance.

Comment on lines 67 to 70
ReactiveFormsModule,
MatMenuModule,
MatSnackBarModule,
ReactiveFormsModule
Copy link
Contributor

Choose a reason for hiding this comment

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

ReactiveFormsModule is listed twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed duplicate entry.

<form class="form-content">
<mat-form-field class="large-field select-project">

<input matInput [matAutocomplete]="project" [formControl]="selectProjectForm.controls['projectName']"
Copy link
Contributor

Choose a reason for hiding this comment

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

HTML code could be neat aligned as mentioned in coding convention. To do so you should install recommended Prettier extension and align it manually or enable alignment on save in vscode settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mpro7 Good point. I use the default "Format Document" or "Format Selection" command in vsc. We had some issues with "Prettier" especially in html files.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kilchenmann what issues exactly? I personally resigned from Prettier, since devs didn't want to extend formatting to some settings. Format Document/Selection depends on the default formatter and defined rules. I think we could revise the coding/formatting convention and provide the best possible solution to automate the rules, as well as get settings aligned.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mdelez mentioned some issues with prettier. I do not remember which ones exactly. And I thought @waychal is using anyway webstorm and not vsc. We should discuss the tools to format code in another task (maybe) on youtrack...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kilchenmann @mpro7 Yes, I use default formatting from webstorm. I will check the conventions in the link provided above by Marcin.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've disabled Prettier. I think we were using the default config file and it was not formatting it the way we wanted. Maybe if we spend some more time on the config file, we can get it to act how we want it to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion has been moved to DSP-936

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@kilchenmann kilchenmann changed the base branch from master to main October 29, 2020 16:50
@kilchenmann
Copy link
Contributor

@waychal I just updated this branch from main branch. It was outdated.
I will review this PR tomorrow again

matInput
placeholder="Select project"
[matAutocomplete]="project"
[formControl]="selectProjectForm.controls['projectName']">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the problem:

ERROR: projects/dsp-ui/src/lib/action/components/select-project/select-project.component.html:9:17 - error >TS2739: Type 'AbstractControl' is missing the following properties from type 'FormControl': registerOnChange, >registerOnDisabledChange, _applyFormState
[formControl]="selectProjectForm.controls['projectName']">

I prefer this way:

<form class="form-content" [formGroup]="selectProjectForm">
    <input
   [formControlName]="'projectName'">

</form>

Copy link
Contributor

@tobiasschweizer tobiasschweizer Nov 20, 2020

Choose a reason for hiding this comment

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

You can reproduce this locally when building in dev mode: npm run build-lib-dev.

This command builds the lib in dev mode (without the --prod flag) and uses

"strictTemplates": true

see https://indepth.dev/a-look-at-major-features-in-the-angular-ivy-version-9-release#strict-template-type-checking

The build failed because the compiler could not determine whether you passed the correct type to [FormControl].

<button
mat-icon-button
matSuffix
*ngIf="selectProjectForm.controls['projectName'].value"
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of selectProjectForm.controls['projectName'] you could assign the instance of FormControl to a member of SelectProjectComponent and access it like this, see

ngOnInit() {
// initialize form control elements
this.valueFormControl = new FormControl(null);
this.commentFormControl = new FormControl(null);
// subscribe to any change on the comment and recheck validity
this.valueChangesSubscription = this.commentFormControl.valueChanges.subscribe(
data => {
this.valueFormControl.updateValueAndValidity();
}
);
this.form = this._fb.group({
intValue: this.valueFormControl,
comment: this.commentFormControl
});

Copy link
Contributor

Choose a reason for hiding this comment

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

then from the template you can directly access the class's member valueFormControl

this.selectProjectForm = this._formBuilder.group({
projectName: new FormControl({
value: '', disabled: false
}) as FormControl
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you need the type assertion FormControl for? Did the compiler complain?

Is the disabled: false needed or could you just do new FormControl('') it the empty string is your initial value?

Copy link
Contributor

Choose a reason for hiding this comment

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

Instantiate a FormControl, with an initial value.
const control = new FormControl('some value');
console.log(control.value); // 'some value'

https://angular.io/api/forms/FormControl#initializing-form-controls

@waychal waychal force-pushed the dsp-869-select-project-component branch from b3d7d5f to d96d514 Compare November 20, 2020 13:12
@waychal
Copy link
Contributor Author

waychal commented Nov 23, 2020

@tobiasschweizer Thanks. All tests are passing now.

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer Thanks. All tests are passing now.

Great, anything I should add to the unit test guideline?

@tobiasschweizer
Copy link
Contributor

tobiasschweizer commented Nov 24, 2020

@waychal I added DSP-1104 to add some missing explanation about different build options and tests being run on github-ci.

@waychal
Copy link
Contributor Author

waychal commented Nov 24, 2020

@waychal I added DSP-1104 to add some missing explanation about different build options and tests being run on github-ci.

may be you can separate tests commands and build commands in separate documents?
OR you can have single document where all the commands added in package.json are explained.

@waychal waychal merged commit 4cb3dd5 into main Nov 24, 2020
@waychal waychal deleted the dsp-869-select-project-component branch November 24, 2020 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants