Skip to content

Conversation

BernardGatt
Copy link
Collaborator

@BernardGatt BernardGatt commented Feb 25, 2025

When the exitClick property is set to true, we add a dismiss listener to modal messages.

Part of: INAPP-12673

@BernardGatt BernardGatt requested a review from a team as a code owner February 25, 2025 10:22
Copy link
Contributor

@matt-frizzell matt-frizzell left a comment

Choose a reason for hiding this comment

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

I'm good with this PR. My one question/suggestion isn't blocking and can be discussed elsewhere.

{
persistent = true
}
if (message.properties.gist.exitClick)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you ever think about pulling out the properties as a local variable? I ask because when I see a ton of references like this, the cognitive load of reading all of this can be lessened with some vars that skip reading a ton of dot notation.

Sometimes (don't know if this is still a JS issue) there is even a performance hit at scale with dot notation that you can shortcut with a local var, though a tradeoff in memory. (I don't think that matters in this case so don't consider this line here)

Copy link
Contributor

@matt-frizzell matt-frizzell left a comment

Choose a reason for hiding this comment

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

Cool. You took it even further than I was looking for with the ternary operators but it definitely has the impact I was hoping for. Clearer read and easier understanding of the logic that matters. In this case it is property setting and not business logic, and this change better represents that (imho)

hasCustomWidth: false
};

const gist = message?.properties?.gist;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@BernardGatt BernardGatt merged commit e2c1822 into develop Feb 27, 2025
1 check passed
@BernardGatt BernardGatt deleted the feature/INAPP-12673 branch February 27, 2025 08:35
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.

2 participants