-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Improve the admin UX for a product's "Available On" field #2704
Conversation
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 tests fail because of the usage if the deprecated icon
helper. I think we can improve the UX even more by adding required: true
to the available_on
field.
<div class="input-group"> | ||
<div class="input-group-prepend"> | ||
<span class="input-group-text"> | ||
<%= icon('fa fa-calendar') %> |
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.
icon
is deprecated (deprecations let tests fail), please use solidus_icon
.
<%= icon('fa fa-calendar') %> | ||
</span> | ||
</div> | ||
<%= f.text_field date_attr, value: datepicker_field_value(f.object.public_send(date_attr)), class: 'form-control datepicker', placeholder: placeholder_attr %> |
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.
Could we add an option to set the required
attribute?
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.
Would making "Available On" input required complicate the product creation process in other ways?
I was hesitant to suggest this for this PR as I think it is another can of worms.
@@ -64,8 +64,10 @@ | |||
<div data-hook="admin_product_form_available_on"> | |||
<%= f.field_container :available_on do %> | |||
<%= f.label :available_on %> | |||
|
|||
<%= render "spree/admin/shared/datepicker", f: f, date_attr: :available_on, placeholder_attr: t('spree.available_on_placeholder') %> |
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.
Would be great to have an option to set the required
attribute.
0143a5f
to
b36d104
Compare
@tvdeyen Thank you for the comments. I wonder if a |
b36d104
to
7eb2c15
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.
I wonder if a required attribute would complicate product creation and product imports? While I agree that it would improve the admin UX it might be a bigger issue than this PR should address.
@benjaminwil it won't complicate imports because the required
attribute would only affect the form field not the database or model itself. But, this could complicate the form in that it now forces the user to choose a available on date. I like the idea, but others may see this differently. Let's keep it like it is now and decide that later on. Thanks for the contribution.
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 find very useful to improve the UX of that field but I think the implementation can be done better.
The placeholder
attribute should not be used to describe what that field will do if it will be populated but more as an example of the format of the data that can be entered, as described here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#attr-placeholder.
I'd rather use a more descriptive tooltip there, we already have those in our backend and users should already be used to understand their purpose.
Maybe we can still use the placeholder to show the expected format (even if maybe with a datepicker it's not so useful) but I think we should change its color to make it super clear that it's not the default (pre-filled) value and will not be saved with the rest of the data entered by the user.
@kennyadsl Thank you for pointing this out. I am not sure what the best way to deal with placeholder text should be. It may be confusing to use a standard date like I have instead added a more descriptive tooltip. |
I think it's much better now, can you please remove the following commits:
which are now useless? Also this one |
This adds a generic datepicker component that has a calendar icon.
This commit makes the "Available On" field in product views clearer by prepending a calendar icon to the field. It no longer looks like a plain old text input.
1f11f94
to
945bbba
Compare
@kennyadsl Thanks for your patience – I should dropped those changes before. I've now dropped those older commits entirely. |
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 @benjaminwil !
Thanks, Ben |
In the
solidus_backend
interface, a product's "Available On" field should be used set to a date.If
available_on
is not set, then the product is not displayed on the storefront.Because it's not technically a required field, it might not be clear to an admin why their new product does not appear on the storefront after it's been created. (The reason is because
available_on
has no value.)This PR addresses the "Available On" UX in the following ways:
$input-placeholder-color
that wasn't being set anywhere, so I set it. This is the color that is now used for any<input>
tags placeholder text across the admin now.