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

IRIS-826 add long inventory list #115

Merged
merged 5 commits into from
Mar 26, 2024
Merged

IRIS-826 add long inventory list #115

merged 5 commits into from
Mar 26, 2024

Conversation

paweltomaszewskisaucelabs
Copy link
Contributor

@paweltomaszewskisaucelabs paweltomaszewskisaucelabs commented Mar 25, 2024

One-line summary

Issue : #1234 (only if appropriate)

Description

Added a new page with longer inventory (didn't want to break other teams tests)

Types of Changes

  • New feature (non-breaking change which adds functionality)

Tasks

List of tasks you will do to complete the PR

  • Created Task 1
  • Created Task 2
  • To-do Task 3

Review

List of tasks the reviewer must do to review the PR

  • Tests
  • Documentation
  • CHANGELOG

Deployment Notes

These should highlight any db migrations, feature toggles, etc.

@diemol
Copy link
Member

diemol commented Mar 25, 2024

Were you able to build and run the app locally? Last time I tried it didn't work.

@paweltomaszewskisaucelabs
Copy link
Contributor Author

I was able to do that after downgrading node and python version

@@ -15,7 +14,8 @@ import Select from "../components/Select";
import "./Inventory.css";
import { BacktraceClient } from "@backtrace-labs/react";

const Inventory = () => {
const Inventory = ({data}) => {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should have this be data = InventoryData with the original import so we default to the current behavior? Would make it require no changes to the existing tests, too.

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 introduced a second route for an inventory with different set of data. In future will probably add lazy loading feature there and I think it will be easier to test with a prop instead of dealing with routes to define which data should be used. With next iteration I can change prop into longData: boolean and deal importing data in only one place.

@paweltomaszewskisaucelabs
Copy link
Contributor Author

@diemol Can you take a look? I need also your approval because you are the code owner.

@diemol diemol merged commit 2bf7829 into main Mar 26, 2024
1 check passed
@diemol diemol deleted the IRIS-826-add-long-inventory branch March 26, 2024 10:28
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