-
Notifications
You must be signed in to change notification settings - Fork 145
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 transactions types #285
base: main
Are you sure you want to change the base?
feat: add transactions types #285
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@developerfred : it seems like the build is failing on this branch, are you addressing it? |
There was a problem hiding this 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.
@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 |
@developerfred is attempting to deploy a commit to the smlXL Team on Vercel. A member of the Team first needs to authorize it. |
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 errorIf you run
Fixing components/KBar/Results.tsx:8So first step would be to fix the error in Since evm.codes/components/KBar/Results.tsx Line 8 in eb75abc
to const results = useMatches() Fixing pages/_app.tsx:38Then fix the issue in (only showing the relevant code entry, but click the link for full context) Lines 13 to 53 in eb75abc
The {
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" />,
}, |
@developerfred do you need any more help here - I'm happy to help so we can get this in a reviewable state 😄 |
2217337
to
f95cb6f
Compare
502886b
to
473370d
Compare
cbef637
to
58bc312
Compare
@developerfred all you need to for the to fix the build error is replace this:
with this: 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. |
d80e3dc
to
9e1231c
Compare
9e1231c
to
8dee601
Compare
Thanks @2xic @dorlevi and @charisra build working now version working https://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. |
@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. |
There was a problem hiding this 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.
let title = 'Instructions' | ||
if (isPrecompiled) { | ||
title = 'Precompiled Contracts' | ||
} | ||
if (isTransactionType) { | ||
title = 'Transaction Types' | ||
} |
There was a problem hiding this comment.
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';
I believe this is a call action to attract contributors |
@developerfred I approved the PR. Is there anything pending before you merge? |
ref #270
new transaction type page
how to test