-
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
feat(notifications): implement notifications table, use backend types… #679
Conversation
Deploying beaconchain with Cloudflare Pages
|
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, I only found nitpicks and a question.
|
||
defineEmits<{ (e: 'openDialog'): void }>() | ||
|
||
const cursor = ref<Cursor>() | ||
const pageSize = ref<number>(10) | ||
const { t: $t } = useTranslation() | ||
|
||
// TODO: replace currentNetwork with selection from NETWORK_SWITCHER_COMPONENT that has yet to be implemented | ||
const { currentNetwork } = useNetworkStore() | ||
const networkId = ref<number>(currentNetwork.value ?? 1) |
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.
It might be more future-proof and consistent with the other .vue files if you write ref<ChainIDs>
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.
sure makes sense
return { data } | ||
}, | ||
) | ||
|
||
export function useNotificationsDashboardStore() { | ||
export function useNotificationsDashboardStore(networkId: globalThis.Ref<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.
remark: maybe ...globalThis.Ref<ChainIDs>) {
for consistency with the other .vue files?
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.
done
frontend/i18n/en.json
Outdated
"participation_rate": "Participation rate < {_value}" | ||
}, | ||
"footer": { | ||
"subscriptions": "Network ({count} Subscription) |Validators ({count} Subscriptions)" |
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 we can use spaces after the |
, so: ... | Validators ({count} Subscriptions)
.
At least in my use cases it did not make a difference on the UI.
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.
added a space
return // in case some query params change while loading | ||
} | ||
isLoading.value = false | ||
if (!isStoredQuery(q)) { |
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.
Can it happen?
You set setStoredQuery(q)
above. Can it be changed by some event before we reach line 45?
As I understand JS after 8 months with it: functions are never interrupted. Async does not mean that it runs in parallel but rather it will run as soon as there is nothing else to do (like apps under Windows 3 in the good old century).
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.
yes it could be changed while the webservice request is fetched.
the code at the await
waits for the ws to return data ... in the meantime the loadNotificationsDashboards could called in the meantime
) | ||
|
||
isLoading.value = false | ||
if (!isStoredQuery(q)) { |
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.
so same question :)
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.
same answer
This PR:
This PR is not: