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

Add level indicators and path to stock location and part category dropdowns #8366

Merged

Conversation

chris-thorn
Copy link
Contributor

This fixes #8350 and makes the stock location dropdown in the new interface closely match the dropdown in the classic interface.

Adds hyphens to indicate hierarchy level, and shows stock location path instead of simply stock location name.

Copy link

netlify bot commented Oct 25, 2024

Deploy Preview for inventree-web-pui-preview ready!

Name Link
🔨 Latest commit f90a82c
🔍 Latest deploy log https://app.netlify.com/sites/inventree-web-pui-preview/deploys/6731c09dd5c12c000880e20a
😎 Deploy Preview https://deploy-preview-8366--inventree-web-pui-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 100 (no change from production)
Accessibility: 86 (no change from production)
Best Practices: 100 (no change from production)
SEO: 78 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

@matmair matmair added enhancement This is an suggested enhancement or new feature User Interface Related to the frontend / User Interface labels Oct 25, 2024
@matmair matmair added this to the 0.17.0 milestone Oct 25, 2024
@SchrodingersGat
Copy link
Member

@chris-thorn the implementation looks good here. Can you please implement the same thing for "part category" too?

@chris-thorn
Copy link
Contributor Author

I hope I've correctly updated this PR with the requested changes!

@chris-thorn chris-thorn changed the title Add level indicators and path to stock location dropdown Add level indicators and path to stock location and part category dropdowns Oct 27, 2024
Copy link

codecov bot commented Oct 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.46%. Comparing base (b4310bf) to head (f90a82c).
Report is 314 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8366      +/-   ##
==========================================
- Coverage   84.47%   84.46%   -0.02%     
==========================================
  Files        1170     1172       +2     
  Lines       53456    53297     -159     
  Branches     1999     2015      +16     
==========================================
- Hits        45159    45017     -142     
  Misses       7769     7769              
+ Partials      528      511      -17     
Flag Coverage Δ
pui 68.44% <100.00%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SchrodingersGat
Copy link
Member

It looks like some of the tests are failing now due to the adjusted naming scheme.

You will need to adjust the following unit tests (in ./src/frontend/tests):

Forms - Stock Item Validation

image

Pages - Index - Scan (StockLocation)

image

In both cases you can see that the expected text has changed, causing the test(s) to fail...

@wolflu05
Copy link
Contributor

But do we really want to have the full path name as name on each level? I mean if the tests expect something different,we change that behavior?

@SchrodingersGat
Copy link
Member

@wolflu05 AFAIK this is only intended to change the rendering in form drop downs - as is the current behaviour in CUI

@wolflu05
Copy link
Contributor

Im not sure what I would prefere, I guess none of both. In the end my goal is to implement a tree picker like I did in CUI for all tree models, and then show all parent levels, when a search query matches only a child. But ok then change this for now.

@chris-thorn
Copy link
Contributor Author

But do we really want to have the full path name as name on each level? I mean if the tests expect something different,we change that behavior?

@wolflu05 I agree that neither is ideal, and that a tree picker would be perfect for this.

Just to clarify my reasoning for this PR: Right now in PUI, if stock location or part category names don't include information about the hierarchy when you search for a location or category, you potentially see multiple identical names but with no indication of which is the correct one. It's only when you hover over it that you can see the hierarchy.

Bay 1
├── Shelf 1
│   └── Bin 1
└── Shelf 2
    └── Bin 1

Searching for Bin 1 will produce two results but will not display information about their parent locations, so you don't know whether you're picking the bin on Shelf 1 or Shelf 2.

Until the tree picker is ready for implementation, it's my feeling that the dropdown should behave like the one in CUI.

@SchrodingersGat Ah, sorry about that - I will update the PR today.

@chris-thorn
Copy link
Contributor Author

I've changed pages/pui_scan.spec.ts but I couldn't work out what needed changing in pui_forms.spec.ts. Electronics Lab is a top-level stock location so shouldn't require any additional path detail... Unless I've misunderstood?

@SchrodingersGat
Copy link
Member

@chris-thorn are you using the interactive window in playwright to determine what specific locators you should be looking for?

@chris-thorn
Copy link
Contributor Author

@chris-thorn are you using the interactive window in playwright to determine what specific locators you should be looking for?

Ah, no - I wasn't aware of that. This is my first time using Dev Containers, Playwright, CI pipelines,and even making feature branches for PRs... I want to contribute but I'm struggling a bit with the learning curve!

Following the docs, I seem to have Playwright installed in the devcontainer. However, when I try and run the tests I get:

(venv) vscode ➜ /home/inventree/src/frontend (feature-stock-location-path) $ npx playwright test --ui
Error: Failed to launch: Error: spawn /home/vscode/.cache/ms-playwright/chromium-1134/chrome-linux/chrome ENOENT
    at ChildProcess.<anonymous> (/home/inventree/src/frontend/node_modules/playwright-core/lib/utils/processLauncher.js:132:14)
    at Object.onceWrapper (node:events:632:26)
    at ChildProcess.emit (node:events:529:35)
    at ChildProcess._handle.onexit (node:internal/child_process:290:12)
    at onErrorNT (node:internal/child_process:477:16)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

The Chrome executable appears to exist in the expected location:

(venv) vscode ➜ ~/.cache/ms-playwright/chromium-1134/chrome-linux $ ls -l
total 445808
...
-rwxr-xr-x 1 vscode vscode 411622376 Nov  8 08:08 chrome
...

Running the command to just install Chrome returns the following:

(venv) vscode ➜ /home/inventree/src/frontend (feature-stock-location-path) $ npx playwright install chrome
Switching to root user to install dependencies...
++ arch
+ [[ x86_64 == \a\a\r\c\h\6\4 ]]
+ [[ ! -f /etc/os-release ]]
++ bash -c 'source /etc/os-release && echo $ID'
+ ID=alpine
+ [[ alpine != \u\b\u\n\t\u ]]
+ [[ alpine != \d\e\b\i\a\n ]]
+ echo 'ERROR: cannot install on alpine distribution - only Ubuntu and Debian are supported'
ERROR: cannot install on alpine distribution - only Ubuntu and Debian are supported
+ exit 1
Failed to install browsers
Error: Failed to install chrome

Any idea what I am missing?

@SchrodingersGat
Copy link
Member

ERROR: cannot install on alpine distribution - only Ubuntu and Debian are supported'

Ah, this might be a real hole in our process. I don't use the devcontainer for frontend testing, and so have not run into this issue.

@matmair @wolflu05 do you have any experience running playwright inside our devcontainer?

This might be another good reason to move to a debian based docker image...

@chris-thorn are you running on windows? My dev setup is under WSL (ubuntu) and might be easier to get setup there?

@chris-thorn
Copy link
Contributor Author

Right - that makes sense. Yeah, I'm running the devcontainer in Windows.

I will have a play with WSL, or just try running directly on an Ubuntu server.

@SchrodingersGat
Copy link
Member

For the purpose of this PR, change line /src/frontend/tests/pui_forms.spec.ts:43 from:

await page.getByRole('option', { name: /Electronics Lab/ }).click();

to

await page.getByText('Electronics production facility').click();

@matmair
Copy link
Member

matmair commented Nov 9, 2024

I have not used playwright within devcontainers

@SchrodingersGat
Copy link
Member

@chris-thorn are you able to make the small fix as outlined above?

@chris-thorn
Copy link
Contributor Author

@chris-thorn are you able to make the small fix as outlined above?

Sorry, was away over the weekend. I've now push the change to this branch.

@SchrodingersGat
Copy link
Member

@chris-thorn nice work, thanks for the contribution :) I hope that you can continue to implement good features like this!

@SchrodingersGat SchrodingersGat merged commit e7cfb4c into inventree:master Nov 11, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an suggested enhancement or new feature User Interface Related to the frontend / User Interface
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Full hierarchy in 'Default Location' dropdown not presented to user
4 participants