Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add transactions types #285

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

developerfred
Copy link

@developerfred developerfred commented Jan 4, 2024

ref #270
Captura de Tela 2024-01-04 às 20 35 42

new transaction type page

how to test

  • check if the page is rendering accordingly

Copy link

vercel bot commented Jan 4, 2024

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

Name Status Preview Comments Updated (UTC)
evm-codes ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 30, 2024 2:09pm

@dorlevi
Copy link
Contributor

dorlevi commented Jan 8, 2024

@developerfred : it seems like the build is failing on this branch, are you addressing it?

Copy link
Contributor

@charisra charisra left a comment

Choose a reason for hiding this comment

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

Few comments, also please check the failed build.

components/ChainSelector.tsx Outdated Show resolved Hide resolved
components/ChainSelector.tsx Outdated Show resolved Hide resolved
components/Reference/columns.tsx Outdated Show resolved Hide resolved
components/Reference/columns.tsx Show resolved Hide resolved
context/ethereumContext.tsx Outdated Show resolved Hide resolved
lib/useActions.tsx Outdated Show resolved Hide resolved
pages/transactions.tsx Outdated Show resolved Hide resolved
pages/transactions.tsx Show resolved Hide resolved
pages/transactions.tsx Outdated Show resolved Hide resolved
@2xic
Copy link
Collaborator

2xic commented Jan 19, 2024

@developerfred do you need any help addressing the feedback / getting the build to complete ?

@developerfred
Copy link
Author

@developerfred do you need any help addressing the feedback / getting the build to complete ?

If you could help that would be great too, I can't see the Vercel logs I'm doing typecheck locally

Copy link

vercel bot commented Jan 20, 2024

@developerfred is attempting to deploy a commit to the smlXL Team on Vercel.

A member of the Team first needs to authorize it.

@2xic
Copy link
Collaborator

2xic commented Jan 22, 2024

@developerfred do you need any help addressing the feedback / getting the build to complete ?

If you could help that would be great too, I can't see the Vercel logs I'm doing typecheck locally

Sure, attached the command for running the typechecker and how to fix them 😄

Let us know if you still have any issues 🙌

Finding the build error

If you run yarn run typecheck you should see the following two build errors (if you don't make sure you ran yarn install first)

brage@brages-MBP developerfred-evm.codes % yarn run typecheck

> evm.codes@0.1.0 typecheck
> tsc --pretty

components/KBar/Results.tsx:8:11 - error TS2339: Property 'results' does not exist on type 'ActionGroup[]'.

8   const { results } = useMatches()
            ~~~~~~~

pages/_app.tsx:38:27 - error TS2322: Type '({ id: string; name: string; shortcut: string[]; keywords: string; section: string; subtitle: string; icon: Element; children: { id: string; name: string; shortcut: string[]; keywords: string; section: string; perform: () => void; subtitle: string; icon: Element; }[]; perform?: undefined; } | { ...; } | { ...; })[]' is not assignable to type 'Action[]'.
  Type '{ id: string; name: string; shortcut: string[]; keywords: string; section: string; subtitle: string; icon: Element; children: { id: string; name: string; shortcut: string[]; keywords: string; section: string; perform: () => void; subtitle: string; icon: Element; }[]; perform?: undefined; } | { ...; } | { ...; }' is not assignable to type 'Action'.
    Type '{ id: string; name: string; shortcut: string[]; keywords: string; section: string; subtitle: string; icon: JSX.Element; children: { id: string; name: string; shortcut: string[]; keywords: string; section: string; perform: () => void; subtitle: string; icon: JSX.Element; }[]; perform?: undefined; }' is not assignable to type 'Action'.
      Types of property 'children' are incompatible.
        Type '{ id: string; name: string; shortcut: string[]; keywords: string; section: string; perform: () => void; subtitle: string; icon: Element; }[]' is not assignable to type 'string[]'.
          Type '{ id: string; name: string; shortcut: string[]; keywords: string; section: string; perform: () => void; subtitle: string; icon: JSX.Element; }' is not assignable to type 'string'.

38             <KBarProvider actions={actions}>
                             ~~~~~~~

  node_modules/kbar/lib/types.d.ts:27:5
    27     actions: Action[];
           ~~~~~~~
    The expected type comes from property 'actions' which is declared here on type 'IntrinsicAttributes & KBarProviderProps & { children?: ReactNode; }'


Found 2 errors in 2 files.

Errors  Files
     1  components/KBar/Results.tsx:8
     1  pages/_app.tsx:38

Fixing components/KBar/Results.tsx:8

So first step would be to fix the error in Results.tsx .

Since useMatches() returns an array we want to switch from

https://github.com/smlxl/evm.codes/blob/eb75abc3fe940276e80f0ae64db9a6dbfad13bbc/components/KBar/Results.tsx#L8

to

const results = useMatches() 

Fixing pages/_app.tsx:38

Then fix the issue in pages/_app.tsx where you added a new entry for the theme

(only showing the relevant code entry, but click the link for full context)
https://github.com/smlxl/evm.codes/blob/eb75abc3fe940276e80f0ae64db9a6dbfad13bbc/lib/useActions.tsx#L13-L53

The Action type wants just the action id in the children field so you should be able to switch this to

    {
      id: 'theme',
      name: 'Theme',
      shortcut: ['t'],
      keywords: 'change theme',
      section: 'Settings',
      subtitle: 'Change application theme',
      icon: <Icon name="palette-line" />,
      children: [
        'theme-light',
        'theme-dark',
        'theme-system'
      ],
    },
    {
      id: 'theme-light',
      name: 'Light Mode',
      shortcut: ['l'],
      keywords: 'light theme',
      section: 'Settings',
      perform: () => setTheme('light'),
      subtitle: 'Switch to Light Mode',
      icon: <Icon name="sun-line" />,
    },
    {
      id: 'theme-dark',
      name: 'Dark Mode',
      shortcut: ['d'],
      keywords: 'dark theme',
      section: 'Settings',
      perform: () => setTheme('dark'),
      subtitle: 'Switch to Dark Mode',
      icon: <Icon name="moon-line" />,
    },
    {
      id: 'theme-system',
      name: 'System Default',
      shortcut: ['s'],
      keywords: 'system theme',
      section: 'Settings',
      perform: () => setTheme('system'),
      subtitle: 'Use system theme settings',
      icon: <Icon name="settings-2-line" />,
    },

@2xic
Copy link
Collaborator

2xic commented Jan 24, 2024

@developerfred do you need any more help here - I'm happy to help so we can get this in a reviewable state 😄

@developerfred developerfred force-pushed the codingsh/add-transactions-types branch 2 times, most recently from 2217337 to f95cb6f Compare January 29, 2024 14:32
@developerfred developerfred force-pushed the codingsh/add-transactions-types branch 6 times, most recently from 502886b to 473370d Compare January 29, 2024 15:28
@developerfred developerfred force-pushed the codingsh/add-transactions-types branch 2 times, most recently from cbef637 to 58bc312 Compare January 29, 2024 15:37
@charisra
Copy link
Contributor

@developerfred all you need to for the to fix the build error is replace this:

children: [
  'theme-light',
  'theme-dark',
  'theme-system'
],

with this:
children: ['theme-light', 'theme-dark', 'theme-system']

in: ./lib/useActions.tsx

Want to give this a try and see if it builds? I can help more if you face more problems. Would really help if you got your local ESLint properly setup, if you haven’t already.

@developerfred developerfred force-pushed the codingsh/add-transactions-types branch 4 times, most recently from d80e3dc to 9e1231c Compare January 29, 2024 20:09
@developerfred developerfred force-pushed the codingsh/add-transactions-types branch from 9e1231c to 8dee601 Compare January 29, 2024 20:13
@developerfred
Copy link
Author

developerfred commented Jan 29, 2024

Thanks @2xic @dorlevi and @charisra build working now

version working https://evm-codes-fmjmeuoa2-developerfred.vercel.app/transactions

@charisra
Copy link
Contributor

Thanks @2xic @dorlevi and @charisra build working now

version working evm-codes-fmjmeuoa2-developerfred.vercel.app/transactions

It's lot letting me preview so can't tell if it's ok @developerfred . I still see errors building in the latest builds. Can you do a check that your latest commits build successfully?

Also, if you're using VSCode, please ensure you have Prettier installed and enabled. It should auto-fix several code formatting errors.

@developerfred
Copy link
Author

@charisra My commits were compiled, I also used prettier, what are the next steps?

@charisra
Copy link
Contributor

@charisra My commits were compiled, I also used prettier, what are the next steps?

Great. I'll review once more, also check the Preview, approve and then you can merge.

Copy link
Contributor

@charisra charisra left a comment

Choose a reason for hiding this comment

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

This is going on the right track, close to be merge-able.
@developerfred please add in the PR description a short explanation about what has been done in this PR and how we (reviewers) can test & confirm that the PR achieves its purpose.

Left a few comments, too, should be easy to address.

Comment on lines +15 to +21
let title = 'Instructions'
if (isPrecompiled) {
title = 'Precompiled Contracts'
}
if (isTransactionType) {
title = 'Transaction Types'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: Can become a one-liner: let title = isPrecompiled ? 'Precompiled Contracts' : isTransactionType ? 'Transaction Types' : 'Instructions';

docs/opcodes/16.mdx Show resolved Hide resolved
docs/opcodes/30.mdx Show resolved Hide resolved
@charisra
Copy link
Contributor

charisra commented Jan 31, 2024

So that's what I'm expected to see?
Screenshot 2024-01-31 at 17 58 01

Just the types, there's not any more info on those, it mentions There is no reference doc for this yet. Why not contribute That's the expected behavior?

@developerfred
Copy link
Author

There is no reference doc for this yet. Why not contribute That's the expected behavior?

I believe this is a call action to attract contributors

@charisra
Copy link
Contributor

charisra commented Feb 5, 2024

@developerfred I approved the PR. Is there anything pending before you merge?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants