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: add auto renewal button #254

Merged
merged 79 commits into from
Nov 17, 2023
Merged

feat: add auto renewal button #254

merged 79 commits into from
Nov 17, 2023

Conversation

irisdv
Copy link
Collaborator

@irisdv irisdv commented May 25, 2023

Close #178

Adds a new button on root domains identity page to toggle auto renew of a domain.

@vercel
Copy link

vercel bot commented May 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
app-starknet-id ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 17, 2023 4:58pm
goerli-app-starknet-id ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 17, 2023 4:58pm

Copy link
Contributor

@fricoben fricoben left a comment

Choose a reason for hiding this comment

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

Good PR ! Should be good after these small changes :)

import React, { FunctionComponent, useEffect, useState } from "react";
import { usePricingContract } from "../../../hooks/contracts";
import styles from "../../../styles/components/wallets.module.css";
import styles2 from "../../../styles/Home.module.css";
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename homeStyles instead of styles2

</span>
</p>
)}
<div className={styles2.cardCenter}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is useful.

Capture d’écran 2023-05-28 à 18 47 02

)}
{identity?.domain_expiry && (
<p className="break-all">
<strong>Auto renewal date :</strong>&nbsp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make it really strong using the className strong in global.css ? Because right now it's not bold

To avoid loosing your domain, you can enable auto renewals so your
domain is renewed automatically. Auto renewals happen one month before
your expiry date is reached.
</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Change text to

Avoid losing your domain and renew it automatically each year one month before it expires! (You can disable this option when you want.)

)}
</span>
</p>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you can show the price like so:

            <p className="break-all">
              <strong>Price :</strong>&nbsp;
              <span>
                {price}/year
              </span>
            </p>

components/identities/actions/identityActions.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@fricoben fricoben left a comment

Choose a reason for hiding this comment

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

After a few hours of testing on the front I have some problems on the build:

desc: I enabled auto renew, everything went great and the transaction passed right after (https://testnet.starkscan.co/tx/0x156a23df13a0aa924b20f783ebec3a07495d841f9cff2c6e49d18f61d47f6f3) but I still have some problems that made me thing that the tx wasn't passed.

1/ I enabled AR but the button ON still appears
2/ The tx is passed and worked but the indexer didn't index the time of the renewal. (I still have the same date displayed)

components/UI/textField.tsx Show resolved Hide resolved
pages/register/[domainToRegister].tsx Show resolved Hide resolved
<Renewal
groups={[
process.env.NEXT_PUBLIC_MAILING_LIST_GROUP ?? "",
process.env.NEXT_PUBLIC_MAILING_LIST_GROUP_AUTO_RENEWAL ?? "",
Copy link
Contributor

Choose a reason for hiding this comment

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

en fait ici on a pas besoin de public mailing list ici juste process.env.NEXT_PUBLIC_MAILING_LIST_GROUP_AUTO_RENEWAL

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mais seulement quand la checkbox auto renewal est checked du coup ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok du coup c'est bon

@irisdv
Copy link
Collaborator Author

irisdv commented Nov 16, 2023

2/ The tx is passed and worked but the indexer didn't index the time of the renewal. (I still have the same date displayed)

I think the same date should be displayed on the autorenewal button before and after the tx, because I precompute it in the button, and when you enable auto renewal this is the date that will be used (30 days before it expires).

@irisdv
Copy link
Collaborator Author

irisdv commented Nov 17, 2023

1/ I enabled AR but the button ON still appears

For this, it should be fixed now but we still have a bit of time between the tx being accepted and the event being indexed

Copy link
Contributor

@fricoben fricoben left a comment

Choose a reason for hiding this comment

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

Some small changes and we'll merge + over test this week-end

</a>
</p>
</div>
) : null}
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete for now (we'll soon add it back with new version)

{identity?.domain_expiry
? `Your domain will be renewed for 1 year on
${timestampToReadableDate(
identity.domain_expiry - 2592000
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess 2592000 is the second equivalent to a month. We need to put that in a const for us to understand easily

<Renewal
groups={[
process.env.NEXT_PUBLIC_MAILING_LIST_GROUP ?? "",
process.env.NEXT_PUBLIC_MAILING_LIST_GROUP_AUTO_RENEWAL ?? "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes !

@fricoben fricoben added ❌ Change request Change requested from reviewer and removed 🔥 Ready for review This pull request needs a review labels Nov 17, 2023
Copy link
Contributor

@fricoben fricoben left a comment

Choose a reason for hiding this comment

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

lgtm

@fricoben fricoben merged commit 0135ef3 into testnet Nov 17, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❌ Change request Change requested from reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic Renewal front-end
3 participants