Add test control #2
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request introduces a new TestControlComponent for managing test execution in the OpenHTF web GUI, focusing on test selection, starting, and aborting functionality.
- Added
test-control.component.ts,html, andscssfiles implementing the new component with dropdown test selection and start/abort buttons - Integrated
<htf-test-control>intostation.component.html, adjusting layout for the new component - Updated
stations.module.tsto declare TestControlComponent and import FormsModule for form controls - Modified
station.component.spec.tsto include TestControlComponent in test suite configuration - Minor formatting improvements in
base.scssandmain.scssfor better code readability
8 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile
| <div [@animateIn]="'in'" *ngIf="testRunning()"> | ||
|
|
||
| <div class="htf-layout-widget-body"> | ||
| <div [innerHTML]="control"></div> |
There was a problem hiding this comment.
logic: Using [innerHTML] can be a security risk if the content is not sanitized. Ensure 'control' is properly sanitized before binding.
| <div [innerHTML]="control"></div> | ||
|
|
||
| <div class="u-font-size-large"> | ||
| <button type="button" class="htf-rounded-button-red u-font-size-large" (click)="abort()"> |
There was a problem hiding this comment.
style: There's an extra space in the class attribute between 'htf-rounded-button-red' and 'u-font-size-large'.
| <button type="button" class="htf-rounded-button-red u-font-size-large" (click)="abort()"> | |
| <button type="button" class="htf-rounded-button-red u-font-size-large" (click)="abort()"> |
| :host ::ng-deep ol, | ||
| :host ::ng-deep ul { | ||
| padding-left: 25px; | ||
| margin: 0; | ||
| } |
There was a problem hiding this comment.
style: Consider using more specific selectors to avoid potential conflicts with other components
| :host ::ng-deep ol, | |
| :host ::ng-deep ul { | |
| padding-left: 25px; | |
| margin: 0; | |
| } | |
| :host ::ng-deep .custom-ol, | |
| :host ::ng-deep .custom-ul { | |
| padding-left: 25px; | |
| margin: 0; | |
| } |
| ul { | ||
| list-style: none; | ||
| padding: 0; | ||
| margin: 0; | ||
| width: 100%; | ||
| max-height: 300px; | ||
| overflow-y: auto; | ||
| border: 1px solid #ccc; | ||
| border-top: none; | ||
| border-radius: 0 0 4px 4px; | ||
| position: absolute; | ||
| background-color: #fff; | ||
| box-shadow: 0 2px 6px rgba(0, 0, 0, 0.1); | ||
| } |
There was a problem hiding this comment.
style: Consider using classes instead of element selectors for better specificity and reusability
| ul { | |
| list-style: none; | |
| padding: 0; | |
| margin: 0; | |
| width: 100%; | |
| max-height: 300px; | |
| overflow-y: auto; | |
| border: 1px solid #ccc; | |
| border-top: none; | |
| border-radius: 0 0 4px 4px; | |
| position: absolute; | |
| background-color: #fff; | |
| box-shadow: 0 2px 6px rgba(0, 0, 0, 0.1); | |
| } | |
| .custom-ul { | |
| list-style: none; | |
| padding: 0; | |
| margin: 0; | |
| width: 100%; | |
| max-height: 300px; | |
| overflow-y: auto; | |
| border: 1px solid #ccc; | |
| border-top: none; | |
| border-radius: 0 0 4px 4px; | |
| position: absolute; | |
| background-color: #fff; | |
| box-shadow: 0 2px 6px rgba(0, 0, 0, 0.1); | |
| } |
| li { | ||
| padding: 10px; | ||
| cursor: pointer; | ||
| transition: background-color 0.3s; | ||
| } |
There was a problem hiding this comment.
style: Consider using a class selector for list items instead of the element selector
| li { | |
| padding: 10px; | |
| cursor: pointer; | |
| transition: background-color 0.3s; | |
| } | |
| .custom-li { | |
| padding: 10px; | |
| cursor: pointer; | |
| transition: background-color 0.3s; | |
| } |
| li:hover { | ||
| background-color: #f2f2f2; | ||
| cursor: pointer; | ||
| } |
There was a problem hiding this comment.
style: Redundant cursor property, already set on line 60
| li:hover { | |
| background-color: #f2f2f2; | |
| cursor: pointer; | |
| } | |
| li:hover { | |
| background-color: #f2f2f2; | |
| } |
| li:hover.selected { | ||
| background-color: #3498db; | ||
| color: #fff; | ||
| } |
There was a problem hiding this comment.
style: Consider combining this with the .selected class to reduce specificity
| li:hover.selected { | |
| background-color: #3498db; | |
| color: #fff; | |
| } | |
| li.selected:hover { | |
| background-color: #3498db; | |
| color: #fff; | |
| } |
| options: RequestOptions | ||
| stationBaseUrl: string |
There was a problem hiding this comment.
style: Consider adding type annotations for these properties
| options: RequestOptions | |
| stationBaseUrl: string | |
| options: RequestOptions; | |
| stationBaseUrl: string; |
| protected http: Http, protected flashMessage: FlashMessageService) { | ||
| let headers = new Headers({'Content-Type': 'application/json'}); | ||
| this.options = new RequestOptions({headers}); | ||
| this.stationBaseUrl = getStationBaseUrl(this.config.dashboardEnabled, this.station); |
There was a problem hiding this comment.
logic: this.station is undefined in the constructor
| this.stationBaseUrl = getStationBaseUrl(this.config.dashboardEnabled, this.station); | |
| if (!this.station) { | |
| throw new Error('Station is required'); | |
| } | |
| this.stationBaseUrl = getStationBaseUrl(this.config.dashboardEnabled, this.station); |
| this.http.get(testsUrl, this.options).subscribe((resp: Response) => { | ||
| this.tests = resp.json().tests; | ||
| this.filteredTests = this.tests.slice(); | ||
| }); |
There was a problem hiding this comment.
logic: Add error handling for the HTTP GET request
| this.http.get(testsUrl, this.options).subscribe((resp: Response) => { | |
| this.tests = resp.json().tests; | |
| this.filteredTests = this.tests.slice(); | |
| }); | |
| this.http.get(testsUrl, this.options).subscribe((resp: Response) => { | |
| this.tests = resp.json().tests; | |
| this.filteredTests = this.tests.slice(); | |
| }, (error: Response) => { | |
| this.flashMessage.error('Failed to load tests', messageFromErrorResponse(error)); | |
| }); |
No description provided.