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

Normalise daoSnapshotENS to lower case #2247

Merged
merged 4 commits into from
Aug 17, 2024

Conversation

mudrila
Copy link
Contributor

@mudrila mudrila commented Aug 14, 2024

Closes #2234

@mudrila mudrila added bug Something isn't working maintenance Keep the lights on labels Aug 14, 2024
@mudrila mudrila self-assigned this Aug 14, 2024
Copy link

netlify bot commented Aug 14, 2024

Deploy Preview for decent-interface-dev ready!

Name Link
🔨 Latest commit 0f7888d
🔍 Latest deploy log https://app.netlify.com/sites/decent-interface-dev/deploys/66bfd10de1c5a00008a4d5e4
😎 Deploy Preview https://deploy-preview-2247.app.dev.decentdao.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@adamgall adamgall left a comment

Choose a reason for hiding this comment

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

Didn't go through the whole PR yet, but saw enough to stop.

Please see my comments! Thank you sir

src/components/Proposals/ProposalInfo.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@DarksightKellar DarksightKellar left a comment

Choose a reason for hiding this comment

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

In the same theme of the ongoing discussion on this PR, I'd much rather the "lower-casing" happens only when it is input from the user via the Decent app.

Thus, as long as the ENS string came from Decent, it should be (and in fact IS) guaranteed to be lower case, so we should not need to lower case them anywhere else in-app. If it happens to be mixed case, then it is most certainly not originally from Decent (these guarantees, of course, are after this PR goes live), and I don't think we have any business insisting that it MUST be lowercased for whoever decided (or whatever) that they'd like their name non-standardised.

@mudrila mudrila force-pushed the fix/#2234-normalize-dao-snapshot-ens branch from 2887094 to fe42cf7 Compare August 15, 2024 18:39
src/components/ui/cards/DAOInfoCard.tsx Outdated Show resolved Hide resolved
src/components/ui/cards/DAONodeInfoCard.tsx Outdated Show resolved Hide resolved
src/hooks/DAO/loaders/snapshot/useSnapshotSpaceName.ts Outdated Show resolved Hide resolved
src/hooks/DAO/loaders/useFractalNode.ts Outdated Show resolved Hide resolved
src/hooks/DAO/loaders/useLoadDAONode.ts Outdated Show resolved Hide resolved
src/hooks/utils/useAvatar.ts Outdated Show resolved Hide resolved
src/models/DaoTxBuilder.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@DarksightKellar DarksightKellar left a comment

Choose a reason for hiding this comment

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

I might be missing something, but is the snapshot input field in these screenshots covered by this PR (or is something else entirely lowercasing those values)?

Safe settings:
Screenshot 2024-08-16 at 14 02 50

Create safe
Screenshot 2024-08-16 at 14 02 26

@adamgall
Copy link
Member

@DarksightKellar i would expect that these two inputs are the two places that this PR should touch. What change are you requesting?

Copy link
Member

@adamgall adamgall left a comment

Choose a reason for hiding this comment

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

@mudrila maybe this is why @DarksightKellar requested changes... does this PR do anything anymore? It doesn't seem like any of the changes here do the thing I was looking for: lowercasing text typed into those two input fields Kelvin screenshotted

@adamgall adamgall merged commit 83ba2f2 into develop Aug 17, 2024
7 checks passed
@adamgall adamgall deleted the fix/#2234-normalize-dao-snapshot-ens branch August 17, 2024 01:35
Copy link

sentry-io bot commented Aug 19, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ ContractFunctionExecutionError: RPC Request failed. //hierarchy View Issue
  • ‼️ ContractFunctionExecutionError: RPC Request failed. //home View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working maintenance Keep the lights on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't allow Snapshot ENS names with capitalization
4 participants