-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
[admin_style_v3] Products table with variants #11123
Conversation
95aa75a
to
8223cd0
Compare
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. |
Hidden behind admin_style_v3 feature toggle.
We will add the products in the next commit.
With ellipsis clipping for long lines.
The .content div provides overall page margins and a max width, and is already nested inside the nav menus (this allows the nav background to fill the full width of the page. The main content area should be structured the same way, so we can have flexibility needed to allow some screens to use the full page. This doesn't seem to change the layout of any screens in the admin interface. Now the table content can stretch full width
* attempt with shadows, but they are between every row: http://jsfiddle.net/d872e3nb/ * another attempt at applying styles to tbody groups http://jsfiddle.net/sb38cLdu/
8223cd0
to
f280881
Compare
Flaky spec keeps coming up on this branch.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! 🤞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! So cool to see this moving forward.
Moving this to Test Ready for a final check.
%div{ :class => "toolbar" } | ||
%ul{ :class => "actions header-action-links inline-menu" } | ||
%li#new_product_link | ||
= button_link_to t(:new_product), "/admin/products/new", { :icon => 'icon-plus', :id => 'admin_new_product' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
#products_v3_page{"data-controller": "productsV3"} | ||
#loading-spinner.spinner-container{"data-productsV3-target": "loading"} | ||
.spinner | ||
= t('.loading') | ||
#products-content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
load = () => { | ||
this.showLoading(); | ||
this.stimulate("Admin::ProductsV3#fetch").then(() => this.hideLoading()); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
};
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, writing my comment, I thought I could do in another (smarter?) way ; See commit 9e5078d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an interesting way of doing it. I don't have an opinion about it. 🤷
@jibees I've added to this PR: |
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 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 ;-) |
Thanks Filipe!
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. |
…actions-dropdown-is-not-well-placed Fix container issues introduced by #11123
What? Why?
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.