-
Notifications
You must be signed in to change notification settings - Fork 8
980 upgrade from angular 18 to angular 19 #993
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #993 +/- ##
==========================================
- Coverage 58.38% 57.54% -0.85%
==========================================
Files 136 136
Lines 3809 3811 +2
Branches 677 674 -3
==========================================
- Hits 2224 2193 -31
- Misses 1471 1513 +42
+ Partials 114 105 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -80,6 +82,9 @@ $context-menu-width: 39px; | |||
.ag-header-cell-resize { | |||
cursor: ew-resize; | |||
border-right: 5px solid var(--slab_table_border_color); | |||
&::after { | |||
display: none; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ag-theme-fresh theme is deprecated, and the new ag-theme-alpine has a little line, an icon on the header cell borders indicating that you can drag to resize the column. To leave the theme like we had it before, I simply hide it.
@@ -140,6 +145,10 @@ $context-menu-width: 39px; | |||
border: 1px dashed var(--primary_light) !important; | |||
} | |||
|
|||
&.ag-cell-wrapper .ag-drag-handle.ag-row-drag { | |||
display: none; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ag-theme-fresh theme is deprecated, and the new ag-theme-alpine has a little line, an icon on the header cell borders indicating that you can drag to resize the column. To leave the theme like we had it before, I simply hide it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can study the option to adopt this UX improvement instead to hide it? @carlosra85
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, for me its ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really great work!
@@ -164,6 +166,7 @@ const clickMenuHeaderOnRow = (fixture: ComponentFixture<GridTestComponent>) => { | |||
}; | |||
|
|||
const clickOption = (fixture: ComponentFixture<GridTestComponent>, option: number) => { | |||
console.log(fixture.debugElement.nativeElement.querySelectorAll('li')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log forgotten
@@ -190,7 +193,7 @@ const clickCloseButton = (fixture: ComponentFixture<GridTestComponent>, buttonId | |||
|
|||
describe('Systelab Grid', () => { | |||
let fixture: ComponentFixture<GridTestComponent>; | |||
|
|||
ModuleRegistry.registerModules([AllCommunityModule]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this?
@@ -41,8 +41,8 @@ export abstract class AbstractApiComboBox<T> extends AbstractComboBox<T> impleme | |||
public abstract getTotalItems(): number; | |||
|
|||
public override refresh(params: any): boolean { | |||
if (this.gridOptions && this.gridOptions.api) { | |||
this.gridOptions.api.setDatasource(this); | |||
if (this.gridOptions && this.gridApi) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed to check gridOptions because now we do not acces to gridOptions.api
You can remove the full if and use this.gridApi?.setGridOption the method only will be executed if this.gridApi is defined.
Same to all the other ifs like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update README.md?
@@ -32,7 +33,6 @@ export abstract class AbstractApiGrid<T> extends AbstractGrid<T> implements IDat | |||
protected abstract getData(page: number, itemsPerPage: number): Observable<Array<T>>; | |||
|
|||
public getRows(params: IGetRowsParams): void { | |||
this.gridOptions.api.showLoadingOverlay(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is showing "Loading" now?
|
||
public override doGridReady(event: any) { | ||
super.doGridReady(event); | ||
this.refresh(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review if this.refresh() is needed
protected getRowSelectionType(): RowSelectionOptions { | ||
return { | ||
mode: this.multipleSelection ? 'multiRow' : 'singleRow', | ||
checkboxes: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in some modes we use checkboxes, is still working with this flag?
Check again methods showLoadingOverlay (deprecated) and showNoRowsOverlay (not deprecated) vs hideOverlay (not deprecated) to know if hideOverlay hides overlay for Loading or only No Rows overlay |
|
||
@HostListener('focus', ['$event']) | ||
onFocus(event: FocusEvent) { | ||
console.log(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log
PR Details
Description
This PR has several changes regarded with the next upgrades:
Upgrade from angular 18 to 19
Upgrade ag-grid from 28 to 33
Fixes and improves
Related Issue
Motivation and Context
How Has This Been Tested
Types of changes
Checklist