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

LF-3761: Implement crop sales expandable content #2938

Merged

Conversation

SayakaOno
Copy link
Collaborator

@SayakaOno SayakaOno commented Oct 31, 2023

Description

  • create CropSale transaction table
  • pass mobileView currencySymbol to ExpandedContent (mobileView is for the future, and the style for the desktop view will not be shown and has not been finalized)

Some other improvements

  • remove unused properties in TableV2 stories
  • remove accessor prop that is incompatible with sorting from V2 table
  • fix ExpandableItem's key. (index was being used, and it was expanding a different transaction after the date range is updated)
  • remove hovered colour of the expandable item when expanding
  • update transactionDate's tag to h4 (the title of the page "Finances" is h3 so I chose h4)

Jira link: https://lite-farm.atlassian.net/browse/LF-3761

Type of change

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

How Has This Been Tested?

  • Passes test case
  • UI components visually reviewed on desktop view
  • UI components visually reviewed on mobile view
  • Other (please explain)

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • 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
  • The precommit and linting ran successfully
  • I have added or updated language tags for text that's part of the UI
  • I have added "MISSING" for all new language tags to languages I don't speak
  • I have added the GNU General Public License to all new files

@SayakaOno SayakaOno added the enhancement New feature or request label Oct 31, 2023
@SayakaOno SayakaOno self-assigned this Oct 31, 2023
@SayakaOno SayakaOno force-pushed the LF-3761/Implement_crop_sales_expandable_content branch 2 times, most recently from 6272d69 to f096918 Compare November 1, 2023 21:37
@SayakaOno SayakaOno changed the base branch from integration to LF-3746/Group_transactions_by_date November 1, 2023 21:37
@SayakaOno SayakaOno changed the base branch from LF-3746/Group_transactions_by_date to integration November 1, 2023 21:38
@SayakaOno SayakaOno force-pushed the LF-3761/Implement_crop_sales_expandable_content branch from f096918 to 49bbd1d Compare November 2, 2023 06:07
@SayakaOno SayakaOno changed the title [WIP: blocked by LF-3746] LF-3761: Implement crop sales expandable content LF-3761: Implement crop sales expandable content Nov 2, 2023
@SayakaOno SayakaOno marked this pull request as ready for review November 2, 2023 06:19
@SayakaOno SayakaOno requested review from a team as code owners November 2, 2023 06:19
antsgar
antsgar previously approved these changes Nov 2, 2023
Copy link
Collaborator

@antsgar antsgar left a comment

Choose a reason for hiding this comment

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

It's working/looking great! 🎉

Comment on lines 75 to 76
let quantityTotal = items.reduce((total, { quantity }) => total + quantity, 0);
quantityTotal += ` ${quantityUnit}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT, but I'd probably keep quantityTotal a constant and make the following line another const named something like quantityWithUnit to make it a bit easier to read and make both immutable

@@ -42,7 +43,7 @@
border-radius: 1px;
}

&:hover {
&:hover:not(.expanded) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I kinda liked the hover effect on the expanded item because it showcases more clearly that the item can be clicked on to collapse, but not sure if Figma calls for anything specific in this situation

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'll confirm with Loïc!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Loïc's input:
It should not change colour on hover when the row is expanded, the reason is that it can be distracting when you process the info inside the expanded row.

Copy link
Collaborator

@kathyavini kathyavini left a comment

Choose a reason for hiding this comment

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

Looks really good! I just saw one mobile view issue when I was trying to investigate the right way to use shouldFixTableLayout.

Edit: Sorry I didn't realize that my branch was out of date! I think you fixed this exact thing last night!

Also I noticed one bug -- code isn't from this PR, but the functionality is:

const componentKey = typeLabel === 'Crop Sale' ? 'CROP_SALE' : transactionType;

typeLabel is translated so this will only work in English. But maybe the componentKeys are due to be revisited in the next PR? I think association of custom crop-associated revenues with this table still has to be set up too, is that right?

Copy link
Collaborator Author

@SayakaOno SayakaOno left a comment

Choose a reason for hiding this comment

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

Thank you for reviewing!
All comments should be addressed, and I fixed the transaction amount format ⬇️ to be "XXX.XX".
Screenshot 2023-11-02 at 2 11 55 PM

Also, I updated mapSalesToRevenueItems to fix the bug Joyce found, and the test file has also been updated.

Re-review would be appreciated, thank you!

@kathyavini

association of custom crop-associated revenues with this table still has to be set up too

True...! Thank you so much for catching this!!! I added cropGenerated to the transaction!

@@ -42,7 +43,7 @@
border-radius: 1px;
}

&:hover {
&:hover:not(.expanded) {
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'll confirm with Loïc!

Copy link
Collaborator

@kathyavini kathyavini left a comment

Choose a reason for hiding this comment

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

I got a bit nervous when I noticed transactions weren't storing crop_generated but you fixed everything so easily!! Looks great!

@kathyavini kathyavini merged commit f2c9b08 into integration Nov 3, 2023
2 checks passed
@kathyavini kathyavini deleted the LF-3761/Implement_crop_sales_expandable_content branch November 3, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants