-
-
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
🚧 Products v3: viewing, searching, filtering & pagination #11163
🚧 Products v3: viewing, searching, filtering & pagination #11163
Conversation
1f6f60a
to
4ab89dc
Compare
4ab89dc
to
d9ba104
Compare
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.
Phew, epic job!
As you're aware, it was hard to review as there's a lot of commits and changes. I've got some suggestions, and various comments. I would have gone ahead and tried these out but I'm afraid I've run out of time today.
I also think it will help to pull out my unrelated commits, particularly because one has an effect outside of the feature toggle that will be worth a quick test (I don't want to break the admin interface again!). I'll go ahead with that, and update this branch (and squash a few fixup commits).
table + .pagination { | ||
margin-top: -18px; | ||
} |
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.
Hmm that's a tricky one.
I think ideally tables should have a wrapper that applies the large margin, and pagination could go inside the wrapper. Let's solve that in a future update.
d9ba104
to
9b92996
Compare
b401a2b
to
0620ebe
Compare
Hi @dacook ! Thanks for this deep review. All the thumbs-up-ed comment you've made have been addressed. I let you resolve the one the want to resolve ;) |
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.
Thanks JB, great work.
I just booted it up in my dev environment and it's so beautiful and fast.. 🤩
I think for a second review, it would be better to have a fresh perspective and look at the overall diff on the Files changed tab, rather than review all the commits.
I've updated the testing notes to point to the acceptance criteria of each issue. I also re-ordered the list based on the order they were created. |
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! 💪 ✨
app/reflexes/products_reflex.rb
Outdated
# rubocop:disable Layout/LineLength | ||
query.merge!(name_or_meta_keywords_or_variants_display_as_or_variants_display_name_or_supplier_name_cont: @search_term) | ||
# rubocop:enable Layout/LineLength |
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.
# rubocop:disable Layout/LineLength | |
query.merge!(name_or_meta_keywords_or_variants_display_as_or_variants_display_name_or_supplier_name_cont: @search_term) | |
# rubocop:enable Layout/LineLength | |
search_key = %w[ | |
name | |
meta_keywords | |
# ... | |
].join("_or_") + "_cont" | |
query.merge!(search_key => @search_term) |
This could also be a constant on the variant model.
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.
👌 75f7e0a
cd95eeb
to
59cb086
Compare
- Increase top menu underline height to 3px - reduce underline width to same width of label
as they are the same, expect the font-size
content (header, navigation, content) on both left and right
+ change height for border bottom on selected/hovered menu
For `admin_style_v3` use `<` and `>` for next and previous link instead of `next` and `previous` string Extract a mixin for a default box-shadow Maybe this needs to be redefined. Let's see how next things goes. When a table is followed by a pagination, remove its margin-bottom + border Finally, design the pagination component Add sorting/pagination module, on top of table We use `cablea_ready.replace`, so need to add `#products-content` id Use a `pagy` partial with reflex action, instead of the legacy one - revert the legacy one to its previous state - in reflex, fetch product with page attribute, 1 by default Move `pagy` into `admin/shared/v3/` to be reusable + use fontawesome icons for next and previous page Remove useless line
23c9286
to
5d4ef5b
Compare
And do not apply `a` style to `.button` element. ie. ``` a:not(.button) ```
OK there are some stuff to fine tune, but as it's behind a feature toggle and this PR is already monstrously huge, let's unblocked this work! @jibees you can merge 💪 |
Hooray! Just to confirm, were the eight issues (listed in the description above, now closed) tested to pass? |
@dacook yes apart from error page display due to fail in the background which I don't know how to reproduce. |
I think this resolves [this discussion](openfoodfoundation#11163 (comment)) I guess we just didn't know [how it works](https://docs.stimulusreflex.com/guide/cableready.html#order-of-operations) before..
I think this resolves [this discussion](openfoodfoundation#11163 (comment)) I guess we just didn't know [how it works](https://docs.stimulusreflex.com/guide/cableready.html#order-of-operations) before..
I think this resolves [this discussion](openfoodfoundation#11163 (comment)) I guess we just didn't know [how it works](https://docs.stimulusreflex.com/guide/cableready.html#order-of-operations) before..
What? Why?
This PR introduce the first bunch of features for the new products V3 page, under the
admin_style_v3
feature toggle.Includes:
What should we test?
There are extensive acceptance criteria on each of the issues listed above (although they behave as you'd expect). The first two had been merged already but haven't yet been tested. Please review each of these, and provide any feedback you might have.
Release notes
Changelog Category: User facing changes, under feature toggle