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

feat(dashboard): Nv 4884 push mini preview #7318

Merged
merged 49 commits into from
Dec 18, 2024
Merged

Conversation

BiswaViraj
Copy link
Member

@BiswaViraj BiswaViraj commented Dec 17, 2024

What changed? Why was the change needed?

Screenshots

Screen.Recording.2024-12-17.at.9.46.12.PM.mov
Expand for optional sections

Related enterprise PR

Special notes for your reviewer

…nnecessary wrappers around custom step controls
…nnecessary wrappers around custom step controls
@BiswaViraj BiswaViraj requested a review from ChmaraX December 17, 2024 16:49
@BiswaViraj BiswaViraj marked this pull request as ready for review December 17, 2024 16:49
);
}

if (previewData?.result.type !== ChannelTypeEnum.PUSH) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we already have guarantees in parent components that this push-preview will only be rendered when the channel type is a PUSH?

If yes, we can remove this conditional.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but this also helps TS narrowing the types

Copy link
Contributor

Choose a reason for hiding this comment

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

hahaha Ok, not a good enough reason but not a blocker as well. If we change the preview DTO to a much flatter alternative it would be much easier for us to fix this issue.

Copy link
Contributor

@ChmaraX ChmaraX left a comment

Choose a reason for hiding this comment

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

I've put the phone used in SMS to public/phones/iphone-sms.svg

maybe we could follow same pattern e.g. iphone-notification.svg

@BiswaViraj BiswaViraj merged commit 7ac6ced into next Dec 18, 2024
34 checks passed
@BiswaViraj BiswaViraj deleted the nv-4884-push-mini-preview branch December 18, 2024 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants