-
Notifications
You must be signed in to change notification settings - Fork 8
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
Work fe1 ljankoschek notification page #1829
Conversation
Pull reviewers statsStats of the last 30 days for popstellar:
|
f21e5cb
to
9796d1f
Compare
9796d1f
to
c033203
Compare
…nott roll_call#create
Quality Gate passed for 'PoP - PoPCHA-Web-Client'Issues Measures |
Quality Gate passed for 'PoP - Be1-Go'Issues Measures |
Quality Gate passed for 'PoP - Be2-Scala'Issues Measures |
Quality Gate passed for 'PoP - Fe1-Web'Issues Measures |
Quality Gate passed for 'PoP - Fe2-Android'Issues Measures |
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
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 work 😄
I added some small comments that you can address here, in a separate PR or not at all - depending how you feel about it :)
const WitnessNotification = ({ notification, navigateToNotificationScreen }: IPropTypes) => { | ||
const messageSelector = useMemo( | ||
() => makeMessageSelector(notification.messageId), | ||
[notification.messageId], | ||
); | ||
const message = useSelector(messageSelector); | ||
const decodedData = message && JSON.parse(message.data.decode()); |
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.
Not blocking for me to accept this PR but just as input for the future: I think this would be a great place to add an extra function that does the content checks, e.g. decodedData.object === 'roll_call' && decodedData.action === 'create'
, and then returns a typed object (or null if it is not supported). You could probably use MessageRegistry.buildMessageData
for this :)
This way you would have type safety + could use toDateString()
for all the dates
marginBottom: 10, | ||
}, | ||
marginB15: { | ||
marginBottom: 15, | ||
}, | ||
marginT10: { | ||
marginTop: 10, |
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.
Is there a particular reason you use this instead of Spacing.x1, Spacing.x2 etc?
<Text style={[Typography.small, styles.boldText]}>Message Information:</Text> | ||
<Text style={Typography.small}>Message ID: {notification.messageId}</Text> | ||
<Text style={Typography.small}>Received from: {message.receivedFrom}</Text> | ||
<Text style={Typography.small}>Channel: {message.channel}</Text> | ||
<Text style={Typography.small}>Sender: {message.sender}</Text> | ||
<Text style={Typography.small}>Signature: {message.signature}</Text> | ||
<Text style={Typography.small}> |
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.
If you use the same style everywhere I think you can wrap the elements inside another elements like here: https://reactnative.dev/docs/text#nested-text. Saves you same duplication :)
Makes Witness Notification page more readable as described in #1773
Currently only roll_call#create messages are actively witnessed (according to https://github.com/dedis/popstellar/blob/master/fe1-web/src/features/witness/network/messages/WitnessRegistry.ts).
If this changes other notification types have to be considered when building this page.