[admin_style_v3] Products table with variants

[dacook]

What? Why?

• Closes [BUU] See products in my catalogue #10694
• Closes [BUU] See variants for my products #11005
• Work in progress on [BUU] As an enterprise user, I want to know if I have products in my catalogue #10679

Developing the new table for displaying and (future) bulk editing of products.

It's a work in progress, so remaining behind feature toggle. Some of the data values have comments for improvements which will be dealt with as we progress (although feel free to comment on these).

There are some changes outside of the feature toggle, however these don't make user-facing changes.

Narrow screen, showing clipping (which is a work in progress):

Wide screen, filling full page width:

What should we test?

No user-facing changes to test (it will be tested when feature toggle is removed).

The placement of a css class in the admin template is moved, so it's possible this affects the layout of some pages. It affected the spec for "New Order Cycle" page, so it would be worth checking this page appears as expected.

Release notes

Changelog Category: 😎 Feature toggle

The title of the pull request will be included in the release notes.

[dacook]

Hmm, this is not rebase-friendly with the dev branch.

So I need to rebase that branch on this one first, to ensure it doesn't mess it up and block further development.

[dacook]

Flaky spec keeps coming up on this branch.

rspec ./spec/system/consumer/caching/shops_caching_spec.rb:22 

[jibees]

I'd say it's fine, but I'm a bit concerned of not merging the whole wip branch, since work is very linked ; I've re-looked at the wip branch, and I didn't find anything that should come with this one, so I'd say: good to go! 🤞

[mkllnk]

Great work! So cool to see this moving forward.

Moving this to Test Ready for a final check.

[mkllnk]

I often wonder why many people don't use the newer hash syntax in Haml: %div{ class: "toolbar" }. Well, in this case, it should probably just be .toolbar.

[mkllnk]

What happened to your opinion that a page should be progressively enhanced by JS? Shouldn't the products be rendered straight away and only be updated by a reflex?

[dacook]

Good question! This is how it was specified in #10679.
I have to admit I hadn't thought through this one. while I prepared the PR it was a combination of both JB and I's work which I didn't think to review myself.

Thinking about it, I agree. Perhaps if it was expensive data to load, we might want to load the rest of the interface first (ie the search and filter selection) before loading the data. But with pagination that shouldn't be a problem.
Also in a more general way, we seek to provide a responsive experience with Turbo.

So.. yeah I'm not sure why we need the loading spinner for the initial load. We do however intend to have it for when you search/filter/paginate.

@jibees, what do you think?

[jibees]

I added a loading spinner as mentionned in the spec. I think it's fine to add such a thing, since once you click on "Search" button, or pagination, interface is blocked by this spinner, and then you cannot click twice. And sometimes yes, it can take some times.

Actually, not sure to follow:

Shouldn't the products be rendered straight away and only be updated by a reflex?

What do you mean?

[mkllnk]

I mean that the initial page render would not show "Loading" but render the products within the document. The "Loading" would only be used for updates when you sear etc.

[jibees]

I personaly prefer to display the "Loading" at startup (as well as updates), just in order to be consistent. But it's not a strong opinion.

[mkllnk]

Maybe it's too early for extraction but I would like to see a more general solution for the "loading spinner. I would expect it to work similar to the disable-with attribute on submit buttons. Is there something pre-existing?

I don't know if it would work but we could have a controller inserting the spinner into the the target element when an action is triggered and then it's replaced with new HTML when the target element is re-rendered.

[jibees]

The controller has changed since, don't know if it fits well for you.

It consists of a loading-controller, that works with event listener.

import ApplicationController from "./application_controller";

export default class extends ApplicationController {
  connect() {
    super.connect();
    document.addEventListener("show-loading", this.showLoading);
    document.addEventListener("hide-loading", this.hideLoading);
  }

  disconnect() {
    document.removeEventListener("show-loading", this.showLoading);
    document.removeEventListener("hide-loading", this.hideLoading);
  }

  hideLoading = () => {
    this.element.classList.add("hidden");
  };

  showLoading = () => {
    this.element.classList.remove("hidden");
  };
}

and then products-controller is simply:

import ApplicationController from "./application_controller";

export default class extends ApplicationController {
  connect() {
    super.connect();
    // Fetch the products on page load
    this.stimulate("Products#fetch");
  }

  beforeReflex() {
    this.showLoading();
  }

  afterReflex() {
    this.hideLoading();
  }

  showLoading = () => {
    const event = new CustomEvent("show-loading");
    document.dispatchEvent(event);
  };

  hideLoading = () => {
    const event = new CustomEvent("hide-loading");
    document.dispatchEvent(event);
  };
}

[jibees]

Actually, writing my comment, I thought I could do in another (smarter?) way ; See commit 9e5078d

[mkllnk]

That's an interesting way of doing it. I don't have an opinion about it. 🤷

[dacook]

@jibees I've added to this PR:

• Closes [BUU] As an enterprise user, I want to know if I have products in my catalogue #10679

[filipefurtad0]

Hey @dacook and BUU team 🙌

I've staged this PR and, after enabling the toggle, I was able to see the page, on the new separator new_products, only as superadmin:

I see no changes on the products tab:

Smoke tested the order cycles page, all seems as before.

My pics differ quite a bit from those you've posted... any idea why? In anycase, as this is a WIP, I'll move to ready-to-go and let you click the merge button ;-)

[dacook]

Thanks Filipe!

My pics differ quite a bit from those you've posted... any idea why?

Sorry for the confusion: "new_products" is the old new products 🤪 . The "new" new products isn't on the menu yet, but should be in the next PR.