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.

Sign up for GitHub

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

Jump to bottom

UX improvements for creation of new products [OFN-12744] #12760

Merged
merged 6 commits into from
Aug 29, 2024
Merged

UX improvements for creation of new products [OFN-12744] #12760

merged 6 commits into from
Aug 29, 2024

Conversation

wandji20
Copy link
Contributor

@wandji20 wandji20 commented Aug 11, 2024 •
edited by dacook
Loading

What? Why?

What should we test?

As per issue

  • The last point (retaining image upload on error) was not covered.
    Implementing a solution that will keep the uploaded image when form validation is done on the server side can be hacky.
    I'm not so sure if we want to do that.
  • All other points were covered

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • Technical changes only

UX improvements for the creation of new products

Dependencies

N/A

Documentation updates

N/A

@wandji20 wandji20 marked this pull request as draft August 11, 2024 21:16
@wandji20 wandji20 marked this pull request as ready for review August 11, 2024 23:16
@wandji20 wandji20 mentioned this pull request Aug 12, 2024
Closed
dacook
dacook requested changes Aug 13, 2024
View reviewed changes
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Great clean up!
± I have a request to unify the supplier dropdown further.

app/models/spree/product.rb Show resolved Hide resolved
app/views/spree/admin/products/new.html.haml Outdated
%span.required *
= f.select :supplier_id, options_from_collection_for_select(@producers, :id, :name, @product.supplier_id), { include_blank: t("spree.admin.products.new.supplier_select_placeholder") }, { "data-controller": "tom-select", class: "primary" }
= f.select :supplier_id, options_from_collection_for_select(@producers, :id, :name, @product.supplier_id), { include_blank: true }, { "data-controller": "tom-select", class: "primary" }
Copy link
Member

Choose a reason for hiding this comment

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

Should the supplier_select_placeholder be retained?

± Actually it would be worth using SearchableDropdownComponent here. This is a template for a tom-select dropdown that has allows searching by typing, which is very helpful when you have a large number to choose from.
It will ensure it is unified with other dropdowns on the Bulk Edit Products page.

app/views/spree/admin/products/new.html.haml Show resolved Hide resolved
@wandji20 wandji20 requested a review from dacook August 14, 2024 07:48
mkllnk
mkllnk approved these changes Aug 15, 2024
View reviewed changes
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Nice work!

@rioug rioug self-requested a review August 20, 2024 00:03
@wandji20 wandji20 force-pushed the wb-OFN-12744 branch 3 times, most recently from cfea5ff to 5c95fe6 Compare August 20, 2024 20:30
@wandji20
Copy link
Contributor Author

There is a failing spec here I haven't been able to figure out why it fails sometimes (although I initially wrote it)
It seems to me the test runs before the UI is updated.
Any suggestions about handling this?

@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Aug 20, 2024 •
edited
Loading

Hey @wandji20 ,

This looks like this flaky spec.

It looks like the failing line (65) is the same as in the issue. So I wonder if rebasing this PR would help? (I've just tried this on my own PR, let's see.) It also fails on another PR of mine.

In any case, I've re-opened the issue , till you confirm the issue is resolved on your side.

@wandji20
Copy link
Contributor Author

Thanks @filipefurtad0 for sharing updates on the issue.

This looks like #12786.

It's exactly thesame

@wandji20 wandji20 requested a review from rioug August 22, 2024 21:13
@rioug
Remove added eventListener on disconnect
ef2856d 
It's good practise to remove added event listener to avoid memory leak
rioug
rioug approved these changes Aug 26, 2024
View reviewed changes
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Looks good, Thanks.

I forgot to finish my review last week, so I went ahead and did the fix myself.

app/webpacker/controllers/trixeditor_controller.js Show resolved Hide resolved
dacook
dacook approved these changes Aug 26, 2024
View reviewed changes
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Great, ready to test!

app/webpacker/controllers/trixeditor_controller.js Show resolved Hide resolved
@drummer83 drummer83 self-assigned this Aug 29, 2024
@drummer83 drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Aug 29, 2024
@drummer83
Copy link
Contributor

Hi @wandji20,
Thank you for following up on this and fixing those issues. I did some testing and here are the results.

Test results

image

Field Expected Before After
Supplier Red color of field's label 'Supplier' on error No red color of field's label on error ❌ ✔️
Unit size Red border on error No red border on error ❌ ✔️
Unit name (if variant unit == item) Unit name is kept after error messages Unit name is not kept after error messages ❌ ✔️
Unit name (if variant unit == item) Red color of label, border and 'can't be blank' text on error No red highlighting at all on error ❌ ✔️
Value (if variant unit != item) Red border on error No red border on error ❌ ✔️
Product category Red border on error No red border on error ❌ ✔️
Shipping category Selection is kept after error message Selection is not kept after error message ❌ ✔️
Image Uploaded image is kept on error (see #9297) Uploaded image is not kept on error ❌ ✔️

Remarks

  • Supplier:
    • String "Select a supplier" is not applied anymore. We can live with that, because we don't have it for other fields as well. But remove the string from en.yml then, if unused elsewhere?
    • String "Search for suppliers" is not applied as placeholder, the dropdown is not searchable. Could make sense to add it.
    • The blank selection is not required, because the field is mandatory.
  • Unit size:
    • No need to have this field searchable (short list only).
    • String "Search for units" is not applied as placeholder.
  • Product category:
    • The blank selection is not required, because the field is mandatory.

Results

All topics of the original issue are solved. 🥳
No problems were found with this implementation. We could merge as is. 💪
I have some questions (remarks) regarding some of the dropdowns. ❓
We can follow up in a separate issue/PR.

Merging! 🚀
Thanks again!

@drummer83 drummer83 merged commit afc4c1e into openfoodfoundation:master Aug 29, 2024
52 checks passed
@drummer83 drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Aug 29, 2024
@drummer83 drummer83 mentioned this pull request Aug 29, 2024
Open
@drummer83
Copy link
Contributor

Created openfoodfoundation/wishlist#521 to follow up.

@mkllnk mkllnk added the user facing changes Thes pull requests affect the user experience label Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rioug rioug rioug approved these changes

@dacook dacook dacook approved these changes

@mkllnk mkllnk mkllnk approved these changes

Assignees

@drummer83 drummer83

@wandji20 wandji20

Labels
user facing changes Thes pull requests affect the user experience
Projects
OFN Delivery board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Admin] UX improvements for creation of new products
6 participants
@wandji20 @filipefurtad0 @drummer83 @mkllnk @dacook @rioug