-
Notifications
You must be signed in to change notification settings - Fork 4
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
dApp commit history #38
Conversation
0xExp-po
commented
Sep 5, 2024
- add get project info function and display project info
- add router to 'commit' or 'register'
- add update config modal and function
- keep display connected wallet
- rebase holder structure and update tailwindcss config file
- add commit history view section
Co-authored-by: Pamphile Roy <[email protected]>
✅ Deploy Preview for tansu canceled.
|
✅ Deploy Preview for testnet-tansu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Thanks, this looks really great! I like the history view 😃 🙌 Starting with a visual review and then I will go through the code in more details once a few things are fixed. Connect wallet
Great fix! Linked to the connect, I noticed that if you select a project, then connect your wallet, the update config button does not appear. You need to navigate back and select again the project. On-chain buttonOut of scope but maybe you can quickly fix that button when navigating back it does not show properly Most importantly, the link is broken as it goes to https://stellar.expert/explorer/undefined/contract/undefined. Looks like the previous update broke it as the live version is also broken. Project nameThere is still a duplicate name here Please just remove the name from the topic component so that it looks like that. The name is enough in the other section. Also please reduce the font weight for the name as it's a bit disproportionate compared to the rest. History viewOk this is cool! 😃
|
|
I've fixed bugs based on your feedback
Add some extra function
|
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.
Thanks, there seems to be a wrong path or missing piece of code as an import is failing. I will be able to really review once I can see the preview. I still left some comments.
import { formatDate } from '../service/utils'; | ||
import type { FormattedCommit } from '../types/github'; | ||
|
||
const commitHistory = await getCommitHistory('tupui', 'soroban-versioning'); |
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.
I am not sure I understand why this is set to 'tupui', 'soroban-versioning'
. We want to fetch the history of the project people would select, not of tupui/soroban-versioning
. This is missing a props or something else so that it points to the GitHub of the selected project (url in the config like in the Project info section).
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.
yes, you're right
that's why I said I used demo data
I've developed function to get dynamic info user selects and testing now
after test, I'll push it too
Thanks for the updates I could check all that. (Scrolling not sure but can be left for the next step on UI.) Then it's just missing the jump to latest hash button and working for any project you set. After that we can merge. |
I've fixed register bug |
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.
Thanks for the updates, LGTM now 🚀