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

Typedef changes #206

Merged
merged 7 commits into from
Sep 27, 2024
Merged

Typedef changes #206

merged 7 commits into from
Sep 27, 2024

Conversation

4manasa
Copy link
Contributor

@4manasa 4manasa commented Sep 26, 2024

No description provided.

@4manasa 4manasa requested a review from samhere06 September 26, 2024 15:22
Copy link
Contributor

@niallriddell niallriddell left a comment

Choose a reason for hiding this comment

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

Reviewed - would like some more descriptive reasons why ts-ignore was needed for feedback to optimus and also comment regarding initialising editConfig object

@@ -102,6 +102,7 @@ class DeferLoad extends BridgeBase {
.getActionsApi()
.showData(this.name, dataContext, dataContextParameters, {
skipSemanticUrl: true,
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you leave a comment as to why this ts-ignore is needed so we can pass this back to core team

@@ -114,13 +115,15 @@ class DeferLoad extends BridgeBase {
// Rendering defer loaded tabs in case/ page context
this.thePConn
.getActionsApi()
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you leave a comment as to why this ts-ignore is needed so we can pass this back to core team

@@ -120,7 +120,8 @@ class ListView extends BridgeBase {
const theConfigProps = this.thePConn?.getConfigProps();
if (theConfigProps) {
this.selectionMode = theConfigProps.selectionMode;
const componentConfig = this.thePConn.getRawMetadata().config;
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you leave an accurate description of the issue. For example:

// @ts-ignore - required because ComponentMetadataConfig does not have config object in type def

@@ -183,6 +183,7 @@ class SimpleTableSelect extends BridgeBase {
parameters: this.parameters
};

// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Reason for ts-ignore please

@tumms2021389
Copy link
Collaborator

@4manasa , how come we missed all these changes in typedefs package upgrade PR??

Copy link
Contributor

@niallriddell niallriddell left a comment

Choose a reason for hiding this comment

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

Remove unnecessary cast to any

@@ -183,7 +183,7 @@ class SimpleTableSelect extends BridgeBase {
parameters: this.parameters
};

const filters = this.thePConn.getRawMetadata().config.promotedFilters ?? [];
const filters = (this.thePConn.getRawMetadata() as any).config.promotedFilters ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace unnecessary cast to any with // @ts-ignore - promotedFilters missing from types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@@ -24,7 +24,8 @@ export class BridgeBase extends LitElement {
@property({ attribute: false, type: String }) validateMessage;
@property({ attribute: false, type: Object }) theComponentStyleTemplate: any = nothing; // Any styling lit-html template that should be added to renderTemplates

@property({ attribute: false, type: Object }) thePConn; // Normalize incoming pConn to a PConnect object
// @ts-ignore
@property({ attribute: false }) thePConn: typeof PConnect; // Normalize incoming pConn to a PConnect object
Copy link
Contributor

@vishalshrm539 vishalshrm539 Sep 27, 2024

Choose a reason for hiding this comment

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

The type of the "pConn" below needs to be defined too since we're using that also in the components and without it being defined, typings won't work in those cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pConn below is not of type PConnect and we are not using much in the components.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should only be referring to thePConn as it goes through normalization (for some reason I still don't quite understand). Any references to this.pConn are likely incorrect outside of BridgeBase.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's quite confusing also. We need to remove these occurrences probably in the future release.

@4manasa 4manasa merged commit d1fcd2b into main Sep 27, 2024
4 checks passed
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.

4 participants