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 placeholder to price cells in product list #4972

Merged
merged 18 commits into from
Jun 25, 2024

Conversation

Cloud11PL
Copy link
Member

@Cloud11PL Cloud11PL commented Jun 18, 2024

What type of PR is this?

  • 💅 Refactor
  • 🌟 Feature
  • 🔥 Bug Fix
  • 🔩 Maintenance
  • 🛠 Workflow CI/CD changes

Related Issues or Documents

  • closes #

Usage Instructions, Screenshots, Recordings

CleanShot 2024-06-18 at 16 16 29

Have you written tests?

  • Yes!
  • No... here is why: Writing tests are mandatory, please replace this text with why test are not included in this PR

[Optional] Description

Copy link

changeset-bot bot commented Jun 18, 2024

🦋 Changeset detected

Latest commit: 96506e5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
saleor-dashboard Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot temporarily deployed to pr-4972 June 18, 2024 11:47 Destroyed
@github-actions github-actions bot temporarily deployed to pr-4972 June 18, 2024 14:16 Destroyed
@Cloud11PL Cloud11PL marked this pull request as ready for review June 18, 2024 14:23
@Cloud11PL Cloud11PL requested a review from a team as a code owner June 18, 2024 14:23
@github-actions github-actions bot temporarily deployed to pr-4972 June 18, 2024 14:25 Destroyed
Copy link
Member

@poulch poulch left a comment

Choose a reason for hiding this comment

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

You could put logic when click price into handleRowAnchor and handleRowClick ane there

Comment on lines 109 to 111
params.append(`0[s0.${ProductFilterKeys.channel}]`, "");

history.replace({ search: params.toString() });
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: This will not work when someone has already selected some filters

@github-actions github-actions bot temporarily deployed to pr-4972 June 19, 2024 14:48 Destroyed
poulch
poulch previously approved these changes Jun 20, 2024
src/products/components/ProductListDatagrid/utils.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pr-4972 June 21, 2024 07:47 Destroyed
poulch
poulch previously approved these changes Jun 21, 2024
@github-actions github-actions bot temporarily deployed to pr-4972 June 24, 2024 07:47 Destroyed
"saleor-dashboard": minor
---

Product list no longer show a dash instead of product's price when channel is not selected. Instead 'Select channel' shortcut was added meaning that you can now use it to open filters window with 'channel' already selected.
Copy link
Member

Choose a reason for hiding this comment

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

It can be simpler and a bit shorter:

You can now open channel selection by clicking "Select channel" within any price cell on the product list. This means the dash "-" sign is no longer displayed there

src/products/components/ProductListDatagrid/utils.ts Outdated Show resolved Hide resolved
const params = new URLSearchParams(search);

const filtersIndices = Array.from(params.keys())
.map(key => {
Copy link
Member

Choose a reason for hiding this comment

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

this whole map can be a dedicated function, the name will indicate what you map to

Comment on lines 99 to 106

const handlePriceClick = (productId: string) => {
if (!productId) return;

history.replace({ search: getPriceClickSearchParams(history.location.search) });

filterWindow.setOpen(true);
};
Copy link
Member

Choose a reason for hiding this comment

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

issue: you just smuggled here the price-related logic, to the component that is already large and mixing a variety of contexts. You can create hook for it instead and use it here to encapsulate that logic

Copy link
Member

Choose a reason for hiding this comment

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

the hook could return for you an adapter as well so the preparation of it also could be moved

@github-actions github-actions bot temporarily deployed to pr-4972 June 24, 2024 13:38 Destroyed
@Cloud11PL Cloud11PL requested a review from poulch June 24, 2024 14:37
@Cloud11PL Cloud11PL enabled auto-merge (squash) June 25, 2024 07:37
@Cloud11PL Cloud11PL merged commit 2c84b7a into main Jun 25, 2024
12 checks passed
@Cloud11PL Cloud11PL deleted the MERX-5-product-list-placeholder branch June 25, 2024 07:41
Copy link

cypress bot commented Jun 25, 2024

1 failed and 2 flaky tests on run #6249 ↗︎

1 238 7 0 Flakiness 2

Details:

Triggered via release - DASHBOARD 3.14.18,
Project: Saleor Commit: 4f86d13823
Status: Failed Duration: 08:02 💡
Started: Jun 25, 2024 3:14 PM Ended: Jun 25, 2024 3:22 PM
Failed  cypress/e2e/products/productsList/filteringProducts.js • 1 failed test • UI - Electron

View Output

Test Artifacts
An uncaught error was detected outside of a test Screenshots
Flakiness  apps.js • 1 flaky test • UI - Electron

View Output

Test Artifacts
As a staff user I want to manage apps > should be able to create app. TC: SALEOR_3001 Screenshots
Flakiness  configuration/shippingMethods/channelsInShipping.js • 1 flaky test • UI - Electron

View Output

Test Artifacts
As a staff user I want have different shipping method prices for each channel > should be able to display different price for each channel. TC: SALEOR_0805 Screenshots

Review all test suite changes for PR #4972 ↗︎

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