-
Notifications
You must be signed in to change notification settings - Fork 35
[MOB-10141] make lint rules stricter #604
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
[MOB-10141] make lint rules stricter #604
Conversation
…arser and plugin, and update rules
… to target src directory
…fety and consistency
…leHtmlInAppContent, and IterableInAppMessage; introduce IterableHtmlInAppContentRaw type for improved data handling
… to improve type safety; update animatedValue type in IterableInbox for consistency
… with 'unknown' and improve parameter types for better consistency
… rules and plugin usage for consistency
…-alpha/MOB-10141-make-lint-rules-stricter
…-alpha/MOB-10141-make-lint-rules-stricter
…ts lint issues from js files.
…and integration-testing from eslintIgnore
…readability and alignment
…ved maintainability and readability and to conform to react-native/no-color-literals suggested rule.
… maintainability and consistency and to fix eslint rule react-native/no-color-literals
…maintainability and consistency and to conform to suggested eslint rules: react-native/no-color-literals and react-native/no-unused-styles
…stency for better maintainability and to fix recommended eslint rules react-native/no-color-literals and react-native/no-unused-styles
…proved maintainability and consistency; update User component styles for better alignment with design standards.
…arity and maintainability; add comments for future improvements.
…-alpha/MOB-10141-make-lint-rules-stricter
…-alpha/MOB-10141-make-lint-rules-stricter
…-alpha/MOB-10141-make-lint-rules-stricter
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.
Some comments and nit picks but otherwise looking good
|
||
const RNIterableAPI = NativeModules.RNIterableAPI; | ||
const RNEventEmitter = new NativeEventEmitter(RNIterableAPI); | ||
|
||
// TODO: Comment | ||
export interface IterableInboxProps { | ||
export interface IterableInboxProps | ||
extends Pick<IterableInboxMessageListProps, 'messageListItemLayout'> { |
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.
What does this change do?
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.
Ah I see. It simplifies the props by extended the other props. Nice!
static fromDict(dict: { | ||
type: IterableInAppTriggerType; | ||
}): IterableInAppTrigger { | ||
return new IterableInAppTrigger(dict.type); |
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.
Should we add to this as follows:
return new IterableInAppTrigger(dict.type ?? IterableInAppTriggerType.immediate)
So that the type of dict falls back to IterableInAppTriggerType.immediate if dict is null or undefined
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.
Nevermind, I think this way is more stricter and controlled.
@@ -130,9 +129,9 @@ export class IterableConfig { | |||
* Android only feature: This controls whether the SDK should enforce encryption for all PII stored on disk. | |||
* By default, the SDK will not enforce encryption and may fallback to unencrypted storage in case the encryption fails. | |||
*/ | |||
encryptionEnforced: boolean = false; | |||
encryptionEnforced = 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.
Good simplifications. Can we do the same for pushPlatform, dataRegion, allowedProtocols, and logLevel?
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.
You can only do this if it's a primitive type (bool, string etc), so unfortunately we can't do that.
@@ -67,7 +67,7 @@ export class IterableInAppMessage { | |||
expiresAt: Date | undefined, | |||
saveToInbox: boolean, | |||
inboxMetadata: IterableInboxMetadata | undefined, | |||
customPayload: any | undefined, | |||
customPayload: unknown | undefined, |
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.
Nice type safety update.
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.
Thanks!
}; | ||
customPayload: unknown; | ||
read: boolean; | ||
priorityLevel: number; |
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.
Seems like this should have a type like was done for IterableHtmlInAppContentRaw.
returnButtonContainer = !isPortrait | ||
? { ...returnButtonContainer, marginLeft: 80 } | ||
: returnButtonContainer; | ||
|
||
const JS = ` | ||
const links = document.querySelectorAll('a') |
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 this can be put into a constant in another file.
|
||
deleteSlider = isPortrait | ||
? deleteSlider | ||
: { ...deleteSlider, paddingRight: 40 }; |
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.
nice refactor
? height - navTitleHeight - tabBarHeight - tabBarPadding | ||
: height - navTitleHeight, | ||
}, | ||
]} |
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.
Nice refactor
marginTop: 0, | ||
paddingBottom: 10, | ||
paddingLeft: 30, | ||
paddingLeft: isPortrait ? 30 : 70, |
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.
I think these can be given constants too.
🔹 JIRA Ticket(s) if any
✏️ Description
Made lint rules stricter, and made the appropriate code changes to reflect this.