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

Allow admin to edit variant option values #1944

Merged
merged 3 commits into from
Jun 9, 2017
Merged

Allow admin to edit variant option values #1944

merged 3 commits into from
Jun 9, 2017

Conversation

dividedharmony
Copy link
Contributor

This PR reverts ce10040 which, on the edit admin product variants form, hid the ability to edit an existent variant's option value from the admin. However, we want maximize the flexibility for admins to edit variants. Thus, we should display this fieldset to the admin.

This reverts commit ce10040. We want
to allow admins to have maximum flexibility when editing variants.
@jordan-brough
Copy link
Contributor

@dividedharmony this doesn't seem to address some of the concerns that @mamhoff raised in his commit.

He mentioned that with the form, an admin:

is able to change the variant's options to those of a variant that already
exists

Is that not still a danger?

@dividedharmony
Copy link
Contributor Author

@jordan-brough Good point. I can add a validation such that OptionValuesVariants are unique within the scope of any given product.

@mamhoff
Copy link
Contributor

mamhoff commented May 23, 2017

@jordan-brough yes that could still happen, but I've since been convinced that admins should have that power. For example, stores that model packaging via variant options might want to change how a variant is packaged (in a bag or a carton) without having to re-create the variant and possibly losing the connection to the previous packaging for analytics.

<% end %>
<fieldset class="no-border-top no-border-bottom">
<% @product.option_types.each_with_index do |option_type, index| %>
<div class="field four columns <%= 'alpha' if index % 3 == 0 %><%= 'omega' if index % 3 == 2 %>" data-hook="presentation">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This goes back to using the skeleton classes. Can this be updated to use the bootstrap classes (as the removed code did)

With the previous revert, this form went back to the old skeleton
CSS classes. This commit changes those classes to the new and
preferred Bootstrap CSS classes.
@dividedharmony
Copy link
Contributor Author

@jhawthorn Thanks for pointing that out! I've switched back over to the Bootstrap way of doing things.

@jhawthorn jhawthorn merged commit 2fb3ca3 into solidusio:master Jun 9, 2017
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