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

Implementing V2 tree page #1788

Open
wants to merge 6 commits into
base: v2
Choose a base branch
from
Open

Conversation

ha0min
Copy link

@ha0min ha0min commented Jun 27, 2024

Description

This PR implements the tree page with the necessary components and Cypress tests. The page includes tree ID and highlighted information such as species and captured number. Note that the tree growth history has be implemented in a separate pr (#1787 ).

Fixes #1767

Type of change

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Screenshots

Before After
"screenshot before" see below

Desktop version:
image

Mobile version
image

How Has This Been Tested?

  • Cypress integration
  • Cypress component tests
  • Jest unit tests

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@ha0min
Copy link
Author

ha0min commented Jun 27, 2024

Hi @dadiorchen,

Could you kindly review this new design and let me know if it works as expected? I've implemented it under the route v2/trees/:id. Do you think it would be better to use the route /trees/:id instead?

@ha0min ha0min marked this pull request as ready for review June 27, 2024 20:40
@dadiorchen
Copy link
Collaborator

@ha0min can you check the reason that the CI test failed above?

To you question, yes, please use /trees/{id} rather v2, the current /trees/{id} will change to /captures/{id}

@dadiorchen dadiorchen requested a review from sam-rice June 27, 2024 21:38
@dadiorchen
Copy link
Collaborator

@ha0min are you in the community on slack?

@ha0min
Copy link
Author

ha0min commented Jun 27, 2024

@ha0min are you in the community on slack?

Yes, with the username Max Cheng.

@ha0min can you check the reason that the CI test failed above?

To you question, yes, please use /trees/{id} rather v2, the current /trees/{id} will change to /captures/{id}

It is wired as I found the test failed with the test moneytree.cy.js and organizations/[organizationid].cy.js, which cannot reproduced on my end:

image

I have also pushed a new commit to change the page route from /v2/trees/{id} to /trees/{id} as suggested.

Copy link

@sam-rice sam-rice left a comment

Choose a reason for hiding this comment

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

Hey Max,

Dadior and I met recently to discuss the v2 domain model, and how the denormalized response object from the tree endpoint in query api may differ from the actual tree db table, the latter you can see in the diagram here. Note that the denormalized tree object is not yet defined.

Going forward, implementing v2 pages/infrastructure will involve rethinking where certain data will come from. For example, the v2 business model now allows a single tree entity to have one or many associated grower entities. In non-technical terms, a tree may be planted by one grower, and taken care of by a different grower at a later date. Therefore, having a grower field on the tree object doesn't make sense for v2, and grower data should instead come from the most-recent capture (which does have a single associated grower) associated with that tree.

For other types of data, we may want to have query api handle the denormalization of data by aggregating network requests to third-party APIs (for turning geographic/geometric data into a country name, for instance) or different Greenstand services (herbarium api for species names/descriptions, for instance) before returning a denormalized response object to the web map client.

Moving forward, these are the types of conversations we'll need to have as a team, both in code reviews and the weekly tech roundtables. I plan on adding some version of the above information to our Gitbook to help future developers understand our approach to v2 implementation.

For now, please take a look at the updated query api documentation for the denormalized tree and capture response objects and do your best to update the tree page and stub/test data for trees/captures. You'll probably need to touch several different pages and tests to make this work, which was my experience when implementing the v2 grower page.

Let me know if you have any questions! Thanks, Max!

@@ -13,13 +14,15 @@ export function getNockRoutes(
grower: {},
planter: {},
capture: {},
treeCaputres: {},
Copy link

Choose a reason for hiding this comment

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

Could you please correct this spelling mistake ("captures") throughout the file?

describe('Tree Page - Mobile Version', () => {
beforeEach(() => {
// Set viewport to a mobile screen size
cy.viewport('iphone-6');
Copy link

Choose a reason for hiding this comment

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

I think that including testing for mobile layouts is a great idea, and something we should consider requiring in all of the project's integration tests, going forward.

The web-map-client mobile layout was designed with iPhone 12 Pro dimensions (390px × 844px). Could you change this .viewport() to use these dimensions instead?

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