-
-
Notifications
You must be signed in to change notification settings - Fork 729
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
Edit new_resource partial #4742
Edit new_resource partial #4742
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.
LGTM. 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.
Hello @myersca1 , welcome to OFN!!!
Thanks for your PR. We really appreciate when new contributors help.
Converting from erb to haml, perfect 👌 we want to convert all erb to haml, we are almost there.
I have 2 comments below but both optional, this PR can go forward as is.
.form-buttons.filter-actions.actions | ||
= button t(:create), 'icon-ok' | ||
%span.or= t(:or) | ||
= button_link_to t(:cancel), collection_url, icon: 'icon-remove' |
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 why the change from link_to_with_icon to button_link_to. What is the differrence between these two methods? Will the user notice any difference?
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 only difference is purely aesthetic - whether or not it looks like a button, as far as I could tell, but maybe I am mistaken.
@@ -0,0 +1,4 @@ | |||
.form-buttons.filter-actions.actions | |||
= button t(:create), 'icon-ok' |
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.
Looking at the code now, to fix the issue, all you needed would be to change the translation calls from Spree.t to t. because spree.actions.create and spree.actions.cancel are not there in the en.yml file but both are under actions.create (Spree.t is similar to t. but adds spree in the namespace).
Here you are moving to root level :create and :cancel. I think it's ok, I would prefer to keep these under the actions namespace, so I'd use t('actions.create') and t('actions.cancel') instead of t(:create) and t(:cancel).
The OR can stay as t(:or).
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.
Thank you very much - that makes complete sense. We went ahead and updated with the change.
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.
Looks good, thanks!!!
This may help for next time:
https://github.com/openfoodfoundation/openfoodnetwork/wiki/Making-a-great-pull-request#keep-your-commit-history-clean-tidy-and-up-to-date
Congrats @myersca1 🎉 |
ah yes welcome @myersca1 ! Where are my manners 🙈 |
What? Why?
Closes #4109
New_resource_links file was referenced in multiple places, but did not exist. The existing .erb file did not work, so a .haml file was created in its place.
What should we test?
Verify that all pages referencing this file work properly with the translations.
Changelog Category: Fixed