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

Integrate ENS Record Setting in UI #256

Merged
merged 6 commits into from
Oct 11, 2023
Merged

Conversation

juliopavila
Copy link
Collaborator

@juliopavila juliopavila commented Sep 26, 2023

Description:

closes #253

Problem:

Currently, there's no direct way in the UI for users to set their ENS record. Users have to manually add their publication hash to their ENS record, as shown in the example: ENS Domain Example. While they can access it at tabula.gg/#/yourCustomEns, and the app will automatically detect the ENS, the process could be more streamlined.

Solution:

This PR integrates the multicall and setText methods from the ENS contract directly into our UI. Users can now easily set their ENS record by adding a button within the publications section. This button will execute all the necessary logic and explain to the user about the underlying processes.

Changes:

  1. App.tsx: Imported EnsProvider from ./services/ens/context.
  2. WalletBadge.tsx: Made several modifications including importing useEnsContext and refactoring the logic.
  3. EnsModal.tsx: New component to handle the ENS modal UI.
  4. SettingSection.tsx: Added logic to link the publication to the user's ENS name.
  5. ens.context.tsx: New context for ENS operations.
  6. useEns.ts: New hook to handle ENS-related operations.

Additional Context:

The goal of this PR is to enhance the user experience by simplifying the setting of ENS records and clarifying what happens in the background.


Reviewer Checklist:

  • Code is clean and follows the project's coding standards.
  • Functionality has been verified and is working as intended.
  • UI/UX is intuitive and matches the design specs.
  • All new dependencies have been declared.
  • PR is linked to the related issue.

Screenshots

image image image image ![image](https://github.com/gnosis/tabula/assets/19823989/4a5136b5-dd9f-4275-bb60-a7b4d86f919d)

@vercel
Copy link

vercel bot commented Sep 26, 2023

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

Name Status Preview Comments Updated (UTC)
tabula ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 11, 2023 2:08pm

@auryn-macmillan
Copy link
Member

Checked out the deploy preview, but it's not showing the new elements to set ENS names.
image

Copy link
Contributor

@asgeir-s asgeir-s left a comment

Choose a reason for hiding this comment

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

Nice work, just one note:
We will probably need some way of getting the user to change chain to mainnet, if they are not already on mainnet, since the ENS record is always set on mainnet.

packages/app/src/services/graphql.ts Outdated Show resolved Hide resolved
packages/app/src/services/ens/hooks/useENS.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@cedricwaxwing cedricwaxwing left a comment

Choose a reason for hiding this comment

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

LGTM, nice work Julio!

@juliopavila
Copy link
Collaborator Author

Thanks, for your feedback @manboy-eth, I already applied the following changes:

  • Changed default to mainnet subgraph, the only "real" ENS registry, as per your suggestion.
  • Replaced 'multicall' with 'setText' for a single call operation, cleaned up the code, and tested functionality.

Please review

Copy link
Contributor

@asgeir-s asgeir-s left a comment

Choose a reason for hiding this comment

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

Nice, much better :)

But did you do anything to handle this?

We will probably need some way of getting the user to change chain to mainnet, if they are not already on mainnet, since the ENS record is always set on mainnet.

When the user for instance has a publication on Gnosis Chain, the user must still set the ENS record on mainnet.

Copy link
Contributor

@asgeir-s asgeir-s left a comment

Choose a reason for hiding this comment

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

Excellent, now we just need some minor changes and I think this is ready to merge.

packages/app/src/components/commons/NetworkModal.tsx Outdated Show resolved Hide resolved
packages/app/src/components/commons/NetworkModal.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@asgeir-s asgeir-s left a comment

Choose a reason for hiding this comment

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

Excellent, now we just need some minor changes and I think this is ready to merge.

Copy link
Contributor

@asgeir-s asgeir-s left a comment

Choose a reason for hiding this comment

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

Excellent, now we just need some minor changes and I think this is ready to merge.

@juliopavila juliopavila merged commit b3019de into tabula-v.2.1 Oct 11, 2023
4 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 11, 2023
@asgeir-s asgeir-s deleted the issue-#253_ens branch November 7, 2023 15:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants