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

APP 327 integrate Tebu factors #2525

Merged
merged 6 commits into from
Nov 5, 2024
Merged

Conversation

wgardiner
Copy link
Contributor

@wgardiner wgardiner commented Oct 31, 2024

Description

Closes: APP-327


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
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

  1. Review UI here https://deploy-preview-2525--regen-marketplace.netlify.app/project/test-terrasos-project-2

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
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

Copy link

netlify bot commented Oct 31, 2024

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit 00d6b67
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/67299eb61deec60008f66e79
😎 Deploy Preview https://deploy-preview-2525--regen-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@wgardiner wgardiner force-pushed the feat-APP-327-integrate-tebu-factors branch from 8c8e629 to 766bfdb Compare October 31, 2024 01:19
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.

Looks very good. Just one nit: looking at the design the cards should be centered on mobile and take available width: https://www.figma.com/design/brkxGV5qNOkZUp0YQl1cxO/Terrasos-Phase-1?node-id=1185-167335&node-type=instance&m=dev

image

figma:

image

@@ -60,8 +60,30 @@ export interface ProjectPageMetadataLD {
'regen:projectQuote'?: ProjectQuote;
'regen:boundaries'?: string;
'regen:landManagementActions'?: NameImageDescription[];

// Terrasos-specific fields
Copy link
Member

Choose a reason for hiding this comment

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

I would assume these to be on chain if the project is on chain rather than in the project page metadata

Copy link
Member

Choose a reason for hiding this comment

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

see suggestion: 111a15b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forget our motivation, but I know we decided to put all the data into project page metadata, rather than putting data into anchored metadata. @S4mmyb was this just to let us release sooner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see suggestion: 111a15b

so should I wait for this PR to be merged then and use it instead?

Copy link
Member

@blushi blushi Nov 4, 2024

Choose a reason for hiding this comment

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

I wanted to keep things consistent with how we’ve organized things so far: if the project is on-chain, store data that isn't purely for display purposes on-chain and that might be class specific; if the project is off-chain, simply use the off-chain project metadata.

We only have one on chain terrasos project for now so this shouldn't be too much of an effort to put the data there, although it should work if in the interest of time, we keep it off chain for now... and put it on chain later.

Copy link
Member

Choose a reason for hiding this comment

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

see suggestion: 111a15b

so should I wait for this PR to be merged then and use it instead?

this is now merged into dev

Copy link
Collaborator

@flagrede flagrede left a comment

Choose a reason for hiding this comment

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

Looks good!

web-components/src/components/cards/TebuCard/TebuCard.tsx Outdated Show resolved Hide resolved
web-components/src/components/cards/TebuCard/TebuCard.tsx Outdated Show resolved Hide resolved
@wgardiner wgardiner force-pushed the feat-APP-327-integrate-tebu-factors branch 2 times, most recently from 0fa15bb to 6cb0539 Compare November 1, 2024 05:01
@wgardiner wgardiner force-pushed the feat-APP-327-integrate-tebu-factors branch from 6cb0539 to d742e3d Compare November 5, 2024 04:21
@wgardiner wgardiner force-pushed the feat-APP-327-integrate-tebu-factors branch from d742e3d to 00d6b67 Compare November 5, 2024 04:27
@wgardiner wgardiner merged commit 6049a68 into dev Nov 5, 2024
14 checks passed
@wgardiner wgardiner deleted the feat-APP-327-integrate-tebu-factors branch November 5, 2024 04:32
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.

3 participants