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

Telegram channel id on ethical metrics implementation #1890

Merged
merged 32 commits into from
Apr 2, 2024

Conversation

mateumiralles
Copy link
Contributor

@mateumiralles mateumiralles commented Mar 14, 2024

Inputs for the telegram id added in the system / notifications section, and also in the onboarding modal of ethical metrics.

Welcome Modal:

  • Added a new feature introduction for Ethical Metrics notifications.
  • Included instructions guiding users to set up Ethical Metrics notifications with their Telegram Channel ID.
  • Introduced an "Update" button within the modal for those users that already had Ethical Metrics enabled to enhance the user experience, allowing users to modify their notification settings conveniently without leaving the modal.
  • Implemented validation logic to ensure the correct format of the Telegram Channel ID input.

System > Notifications:

  • Introduced a new input field for users to input their Telegram Channel ID.
  • Modified the UI to accommodate the new input field seamlessly within the Ethical Metrics notifications section.
  • Implemented validation logic to ensure the correct format of the Telegram Channel ID input.

These changes aim to empower users with more control over their notifications while maintaining privacy and enhancing the overall user experience.

Screenshot 2024-03-14 at 1 48 07 PM Screenshot 2024-03-14 at 1 48 30 PM

@mateumiralles mateumiralles requested a review from a team as a code owner March 14, 2024 12:42
@github-actions github-actions bot temporarily deployed to commit March 14, 2024 12:43 Inactive
Copy link

github-actions bot commented Mar 14, 2024

Copy link

github-actions bot commented Mar 14, 2024

DAppNode bot has built and pinned the release to an IPFS node, for commit: 291bb63

This is a development version and should only be installed for testing purposes, install link

/ipfs/QmWes2Cpt4DFjmKyv2jXPgNf3PePoPCAZgRpnXe5JzLKmT

(by dappnodebot/build-action)

try {
setValidationMessage("Enabling ethical metrics...");
await api.enableEthicalMetrics({
mail: mailValue || mail,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is weird, use just one value

setValidationMessage("Enabling ethical metrics...");
await api.enableEthicalMetrics({
mail: mailValue || mail,
tgChannelId: tgChannelIdValue || tgChannelId,
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above


async function enableEthicalMetricsSync({
mailValue,
tgChannelIdValue
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using the useStates instead of function arguments

await api.enableEthicalMetrics({
mail: mailValue || mail,
tgChannelId: tgChannelIdValue || tgChannelId,
sync: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you test the behaviour of setting this with option sync to true? Take into account that this might be the onboarding and might take some time the user would not like to wait during setting up dappnode

})
}
variant="dappnode"
disabled={
Copy link
Contributor

Choose a reason for hiding this comment

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

could you try to simplify this condition?

Copy link
Contributor

Choose a reason for hiding this comment

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

it looks a bit complex

) : (
<SwitchBig
disabled={
(tgChannelId === "" && mail === "") ||
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

} catch (e) {
setReqStatusDisable({ error: e });
console.error("Error on registerEthicalMetrics", e);
}
}

function enableEthicalSwitch() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont get this, its useless to have a function that just calls another function. Remove it if possible

Update
</Button>
) : (
<></>
Copy link
Contributor

Choose a reason for hiding this comment

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

Dont add an empty element, instead use above the operator ethicalMetricsOn && ( )

Copy link
Contributor

Choose a reason for hiding this comment

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

TypeScript can have issues inferring the correct type. Using undefined instead of an empty fragment is often clearer in terms of intent maybe?

@github-actions github-actions bot temporarily deployed to commit March 15, 2024 11:42 Inactive
@github-actions github-actions bot temporarily deployed to commit March 15, 2024 12:28 Inactive
@github-actions github-actions bot temporarily deployed to commit March 15, 2024 13:13 Inactive
@github-actions github-actions bot temporarily deployed to commit March 20, 2024 09:25 Inactive
@github-actions github-actions bot temporarily deployed to commit March 20, 2024 09:25 Inactive
@github-actions github-actions bot temporarily deployed to commit March 26, 2024 11:07 Inactive
@github-actions github-actions bot temporarily deployed to commit April 1, 2024 07:37 Inactive
@pablomendezroyo pablomendezroyo merged commit bcc14e4 into develop Apr 2, 2024
8 checks passed
@pablomendezroyo pablomendezroyo deleted the mateu/telegram-channel-id-input branch April 2, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants