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

feat: add featured projects to registry home #49

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

flagrede
Copy link
Collaborator

@flagrede flagrede commented Oct 23, 2023

Description

Part of: regen-network/rnd-dev-team#1811

  • add homePageProjectsSection
  • update homePage document

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • added new items content to ./deskStructure.js
  • go through "Deploying to production" instructions after this PR (and other PRs depending on this one, e.g. on regen-network/regen-web, if applicable) get merged

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • reviewed documentation is accurate
  • manually tested (if applicable)

@flagrede flagrede requested a review from a team October 23, 2023 13:52
of: [
{
type: 'reference',
to: [{ type: 'project' }],
Copy link
Member

Choose a reason for hiding this comment

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

I believe we don't necessarily want a reference to an existing project in Sanity because this means the editor will need to provide project image, location, etc. if the project doesn't already exist while we can easily get this from the marketplace app (contrary to the website at this point), so I guess just a list of strings could be sufficient.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using a list of strings here would prevent editors from reusing previously used IDs. Searching a project by name is also a better experience in sanity than having to input an ID manually. By doing so we also capitalize on a single source of truth for a project that could be helpful elsewhere.
There are currently 4 mandatory fields for the project entity in sanity, among those I think we could only keep name and id as mandatory making the project creation almost the same effort as using an id directly but with additional benefits.

Copy link
Member

Choose a reason for hiding this comment

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

yeah then if we only make name and id required, I agree that works better

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's done

Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -17,7 +17,7 @@ export default {
},
{
name: 'projectsSection',
type: 'titleCustomBody',
type: 'homePageProjectsSection',
Copy link
Member

Choose a reason for hiding this comment

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

just a side note that this is breaking and leads to the following error on https://dev.app.regen.network/ and all deploy previews that do not include your code from regen-network/regen-web#2191 until this is merged
image

so it might be best to make use of sanity graphql tagged endpoints: https://www.sanity.io/docs/graphql#e2e900be2233

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't aware we could create additional tags like that, will make more use of that in the future if needed.

Copy link
Member

Choose a reason for hiding this comment

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

I'll create an issue to add a note in the README about that

@flagrede flagrede merged commit cd79eb3 into main Oct 31, 2023
@flagrede flagrede deleted the feat-1811-select-featured-projects branch October 31, 2023 10:07
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.

2 participants