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

Gardens Metadata: GitHub Api Integration #519

Merged
merged 10 commits into from
Jul 16, 2021
Merged

Gardens Metadata: GitHub Api Integration #519

merged 10 commits into from
Jul 16, 2021

Conversation

rperez89
Copy link
Member

@rperez89 rperez89 commented Jul 4, 2021

Couple of comments:

Created this repo test that we can use for you guys to test how this works and checkout the commits, formats etc
https://github.com/1Hive/dao-list-test
The idea that I have is after you guys validate this process is to create the real repo that or modify the existing one, and create a 1hive-gardens GitHub user that is going to be the one that will be committing to master from the frontend and that user Github token is going to be set on the REACT_APP_GITHUB_API_TOKEN env variable.

The gardens-dao-list repo is going to be configured in a way where the only user allowed to commit directly to master is 1hive-gardens, this way we work in an optimistic way and users that creates gardens have they metadata available as soon as they launch it, if the community see that some of that metadata break our covenant a PR to gardens-dao-list would be needed to remove the dao from the list.

Note that I left a Test button on the screen so you can execute the process since this github publish is going to be done right after the dao is deployed and we get the dao address back since is needed to be included on the dao data.

Also the real gardens-dao-list will include the test that we had in the old one so when a dao is removed those test can be run to check all is ok

Each dao entry will generate one commit per asset and one commit into the network file to include the data, the commits will include the Dao name on the commit message so we can have an easy file history to read

@vercel
Copy link

vercel bot commented Jul 4, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/1hive/honey-pot/9NXG38PqQ1AqiW4b5m2LNh7Wu7w4
✅ Preview: https://honey-pot-git-github-api-1hive.vercel.app

@rperez89 rperez89 changed the base branch from master to gardens July 4, 2021 03:03
@rperez89 rperez89 requested review from fabriziovigevani and 0xGabi and removed request for fabriziovigevani July 4, 2021 03:13
0xGabi
0xGabi previously approved these changes Jul 5, 2021
@0xGabi 0xGabi dismissed their stale review July 5, 2021 23:09

I approve it by mistake. I didn't finish the review yet.

Copy link
Member

@fabriziovigevani fabriziovigevani left a comment

Choose a reason for hiding this comment

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

Really awesome work Ro! I think the process makes sense, really well thought out!

Left some specific comments. Let me know what do you think.

As general corrections:

  • We need to make the images available when coming back to this screen. As it is currently, images are not displayed on that scenario.
  • Logo types are usually rectangular since they involve logo and letters, so we shouldn't have the container circled for that case.
  • Noticed that when clicking inside the asset container but outside one of the logo containers, the window to select a file pops up. This might be due to some styling issue.
  • Noticed that we are adding the garden type value to the dao-list which i don't think we want to do that as we get that form the subgraph.
  • We are missing all from validations. (name not empty, description not empty, valid urls, etc)

src/components/Onboarding/index.js Outdated Show resolved Hide resolved
src/components/ImageUploader.js Outdated Show resolved Hide resolved
src/components/Onboarding/Screens/GardenMetadata.js Outdated Show resolved Hide resolved
src/components/Onboarding/Screens/GardenMetadata.js Outdated Show resolved Hide resolved
src/components/Onboarding/Screens/GardenMetadata.js Outdated Show resolved Hide resolved
src/components/Onboarding/Screens/GardenMetadata.js Outdated Show resolved Hide resolved
src/components/Onboarding/Screens/GardenMetadata.js Outdated Show resolved Hide resolved
src/components/Onboarding/Screens/GardenMetadata.js Outdated Show resolved Hide resolved
src/services/github.js Outdated Show resolved Hide resolved
src/providers/Onboarding.js Show resolved Hide resolved
Copy link
Member

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

Well done Ro! 🙌

I left a few final comments and suggestions to add before merging.

src/components/Onboarding/Screens/GardenMetadata.js Outdated Show resolved Hide resolved
src/components/Onboarding/Screens/GardenMetadata.js Outdated Show resolved Hide resolved
src/components/Onboarding/Screens/GardenMetadata.js Outdated Show resolved Hide resolved
Copy link
Member

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

Looking really good now!

src/components/Onboarding/Screens/GardenMetadata.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants