-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Space admins can create "Menu/Product Groups" and reorder them for display on their marketplace #2361
Conversation
7db7d3c
to
757a0fc
Compare
@zinc-collective/convene-maintainers I'm going to pause dev on this spike for the moment for a gut-check from the committee. 🙏🏻 |
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 think this is a great start! I left some comments (I had actually read this before I left last weekend but completely forgot to click the submit! Apologies!)
e3e0591
to
60b12fc
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.
This is great! Ship it!
`Marketplace::Product`, `without_group_tag`, implemented in terms of the `Tag.group_tag` scope. Add specs for this new scope.
Bumps [square.rb]() from 37.0.0.20240417 to 38.0.0.20240515. --- updated-dependencies: - dependency-name: square.rb dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [positioning](https://github.com/brendon/positioning) from 0.2.1 to 0.2.2. - [Changelog](https://github.com/brendon/positioning/blob/main/CHANGELOG.md) - [Commits](brendon/positioning@v0.2.1...v0.2.2) --- updated-dependencies: - dependency-name: positioning dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [rexml](https://github.com/ruby/rexml) from 3.2.6 to 3.2.8. - [Release notes](https://github.com/ruby/rexml/releases) - [Changelog](https://github.com/ruby/rexml/blob/master/NEWS.md) - [Commits](ruby/rexml@v3.2.6...v3.2.8) --- updated-dependencies: - dependency-name: rexml dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [standard](https://github.com/standardrb/standard) from 1.35.1 to 1.36.0. - [Release notes](https://github.com/standardrb/standard/releases) - [Changelog](https://github.com/standardrb/standard/blob/main/CHANGELOG.md) - [Commits](standardrb/standard@v1.35.1...v1.36.0) --- updated-dependencies: - dependency-name: standard dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Zee <[email protected]>
Bumps [aws-sdk-s3](https://github.com/aws/aws-sdk-ruby) from 1.150.0 to 1.151.0. - [Release notes](https://github.com/aws/aws-sdk-ruby/releases) - [Changelog](https://github.com/aws/aws-sdk-ruby/blob/version-3/gems/aws-sdk-s3/CHANGELOG.md) - [Commits](https://github.com/aws/aws-sdk-ruby/commits) --- updated-dependencies: - dependency-name: aws-sdk-s3 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* --- updated-dependencies: - dependency-name: rubocop-rails dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Fixes new cop warning --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ross Chapman <[email protected]>
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.
Code-wise this looks good, thank you! I'm fine with this being merged once you can get the build green and the rebasing on latest main
sorted out.
My main comment is that I think we should improve the naming and description of "tag groups" and "tags" in user-facing UIs and strings. The goal is to make this as clear to users as possible. But we can make these changes in follow-up PRs.
Calling it a "menu group" doesn't make sense to me (it is not a group of menus). I think some better names could be "product group" or "menu" or maybe "menu tag".
We could either differentiate "tags" from "menus" more clearly in the management UI (maybe even having two separate edit/create places for the two different kinds), or we could instead lean into it and call them "menu tags", and describe them as "special tags that allow you to group a set of your products and control the order in which they are displayed".
<div class="flex flex-col gap-4"> | ||
<div> | ||
<h1>Menu Groups</h1> | ||
<%- if marketplace.tags.group_tag.empty? %> |
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 like how in the "Tags" section there is a helpful message ("Adding tags to your products like "vegan" or "discounted" will help shoppers more easily find what they want"). Let's add something similar here, e.g: "Use product groups like "featured" or "produce" to display some of your products together, and to control the order in which products are displayed." (Or some better wording.)
@@ -30,7 +32,15 @@ | |||
description: "A marvelous marketplace for magic merchandise.", | |||
hero_image: FactoryBot.create(:media) | |||
) | |||
FactoryBot.create(:marketplace, :full, room: marketplace_section) | |||
marketplace = FactoryBot.create(:marketplace, :ready_for_shopping, product_quantity: 16, room: marketplace_section) |
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 think this code exists in main
, and should not be appearing as a diff in this PR.
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.
Ah, it's not actually. You made these changes in this branch with cd5cd4b.
@anaulin Agree about cleaning up the copy. April might even have some insight on what language feels more natural from a user perspective. |
Asking April for input is a good idea! |
Oh, sorry, I forgot that was in this PR! 🤦🏼♀️ |
Foundational work for #2340.
✋🏻✋🏻✋🏻 Let's get these PRs merged first to reduce cognitive load for reviewers:
is_group
andposition
column to Marketplace Tags #2391Tasks
is_section
attribute toMarketplace::Tag
(db, model, controller, etc..)Tag
editing (view, controller, etc...)slot
)Follow-up tasks (2/N)