-
Notifications
You must be signed in to change notification settings - Fork 582
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(onyx-356): single activity panel notification screen #9327
Conversation
This PR contains the following changes:
|
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.
LGTM overall!
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.
LGTM - nice PR!
size={rowImageWidth} | ||
artworkHref={artworkNodes[0].href!} | ||
/> | ||
<Spacer x={`${IMAGE_OFFSET}px`} /> |
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.
Q: any reason for not using x={1}
here?
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.
My thinking was that I have a constant IMAGE_OFFSET (in pixels), and I do all math in this component based on this constant, for instance rowImageWidth
is calculated like this: (width - (TOTAL_HORIZONTAL_PADDING + IMAGE_OFFSET)) / 2
.
If I were to change IMAGE_OFFSET to 15 pixels, for instance, nothing would be broken
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.
but I'll try to rewrite with Flex gap instead, so never mind 😄
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 for explaining 🙏
6d96179
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 Nikita 🙏
This PR resolves ONYX-356
Description
I've added a new screen for individual activity panel notifications. You can navigate to it by clicking on an activity panel item from activity panel or latest activities rail.
Demo
demo.mp4
PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.