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

Feat - Variant Generation #1488

Merged
merged 34 commits into from
Jan 29, 2024
Merged

Feat - Variant Generation #1488

merged 34 commits into from
Jan 29, 2024

Conversation

alecritson
Copy link
Collaborator

This PR looks to add variant generation to the filament hub. This isn't a port of the current offering in 0.7 but is instead a complete reimagining of how it should work, inspired by some other popular systems out there.

Copy link

vercel bot commented Jan 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lunar-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 29, 2024 3:08pm

@glennjacobs
Copy link
Contributor

If there are no shared options, it just shows an empty dropdown list. Maybe we should show a message to say they don't have any set-up?

@glennjacobs
Copy link
Contributor

I don't think we need to say "Restricted" in the add option button, I think that's implied by having "shared" on the other button.

@glennjacobs
Copy link
Contributor

image

The input fields don't say what they are. In BigCommerce they make it clear which is the Name and which are the Values.

image

Also, does our layout allow for "option types" in the future?

@glennjacobs
Copy link
Contributor

image

The "add option" link feels a bit untidy next to the "add value". Maybe it needs to be presented as a typical button to define it more and add some spacing or a rule after, before the save buttons? Again, BigCommerce seem to have this UI working well, so we can take influence from it.

@glennjacobs
Copy link
Contributor

Once variants have been generated (and saved), you can't delete any.

@glennjacobs
Copy link
Contributor

glennjacobs commented Jan 22, 2024

image

The is too much padding on the left, and not enough on the right of each row. And too much space is given to the price and stock, which could probably be better used in the option name and SKU columns.

@glennjacobs
Copy link
Contributor

No more shared options are available.

This suggests that a shared option has already been used, but I don't have any. Maybe we just remove the "more" word?

@glennjacobs
Copy link
Contributor

Removing all values for an option causes this error
image

I'm not sure how you are supposed to completely remove an option and all it's values?

@glennjacobs
Copy link
Contributor

I'm not sure if this should be included, but I can't make a shared option due to the following...

image

@alecritson
Copy link
Collaborator Author

@alecritson there is a little bug on dark mode

Capture d’écran 2024-01-24 à 15 00 06

Thanks for the heads up!

@alecritson alecritson marked this pull request as ready for review January 24, 2024 14:16
@glennjacobs
Copy link
Contributor

If I remove all the variant options I get the following error

image

@glennjacobs
Copy link
Contributor

Maybe for shared options we change the wording to "Remove Shared Option" so it doesn't sound so scary? This would help make it more obvious that it's a shared option also.

image

@glennjacobs
Copy link
Contributor

If I remove all options and save the variants, I have to refresh to get the side menu to show all the sections again.

… feat/variant-editing

# Conflicts:
#	packages/admin/src/Filament/Resources/ProductResource/Widgets/ProductOptionsWidget.php
@alecritson alecritson merged commit d8bbaee into 1.x Jan 29, 2024
15 checks passed
@alecritson alecritson deleted the feat/variant-editing branch January 29, 2024 15:19
@wychoong
Copy link
Contributor

wychoong commented Mar 1, 2024

@alecritson the introduction of product_product_option table is it only for managing variants in filament? do we need to change the way we fetch variants?

@alecritson
Copy link
Collaborator Author

@wychoong This is to make it A) Easier to fetch which options a product has and B) Because we now have local product options which can be specific to a product.

Before you would have to load all variants a product has, load all their product option values, fetch the option itself and then dedupe them it was quite the adventure. Now it can be simplified as such:

$productOptions = $product->productOptions()->with([
    'values' => function ($query) use ($product) {
        $query->whereHas('variants', function ($relation) use ($product) {
            $relation->where('product_id', $product->id);
        });
    },
])->get();

@wychoong
Copy link
Contributor

wychoong commented Mar 1, 2024

@wychoong This is to make it A) Easier to fetch which options a product has and B) Because we now have local product options which can be specific to a product.

Before you would have to load all variants a product has, load all their product option values, fetch the option itself and then dedupe them it was quite the adventure. Now it can be simplified as such:

$productOptions = $product->productOptions()->with([
    'values' => function ($query) use ($product) {
        $query->whereHas('variants', function ($relation) use ($product) {
            $relation->where('product_id', $product->id);
        });
    },
])->get();

Noted. Mainly is because I have a process that create products outside of the provided product page. So now I need to update my code to cater insert to product_product_option, else it won’t show the variants in panel

@ryanmitchell
Copy link
Contributor

Just a heads up that since this PR merged you can no longer make variants on an entirely new product in the hub. You get an error that Field 'tax_class_id' doesn't have a default value:

CleanShot 2024-12-09 at 16 02 57@2x

If you fix that up, you then get an error on the next line as theres no $basePrice as theres no existing variant.

@glennjacobs
Copy link
Contributor

@alecritson Could you have a look in the morning please?

@alecritson
Copy link
Collaborator Author

@ryanmitchell Are you able to provide some steps on how to reproduce this one? I've added a loom video showing what I did and it seemed to generate the variants fine 🤔

https://www.loom.com/share/8c90d6cdf4014d25ad599d80c5048262

@ryanmitchell
Copy link
Contributor

@alecritson thanks for looking, you're right, it works fine when there is a base variant. The issue comes when you remove the base variant and try to add another. Maybe its not something you need to handle, but clients do strange things.

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.

5 participants