-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add ability to translate image_title and image_alt fields #2965
Conversation
e6147c3
to
d50ecad
Compare
@@ -19,6 +19,12 @@ def thumbnail_urls(image) | |||
|
|||
thumbnail_urls | |||
end | |||
|
|||
# We show the title from the next available locale | |||
# if there is no title for the current locale |
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'm not sure if this is what users wants everytime.
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.
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.
So, this method is not good too : https://github.com/refinery/refinerycms/blob/master/pages/app/helpers/refinery/admin/pages_helper.rb#L50 ?
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.
Do you have any clue to fix this?
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.
We do that on the backend because otherwise it is simply blank, but we don't want to display wrong content on the frontend. Maybe we should write the pages one better too.
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.
At this time we only use image_title_with_translations
method in backend.
7dbb04c
to
180446e
Compare
let(:gridview_title_selector) {'[tooltip]'} | ||
let(:gridview_alt_selector) {'[alt]'} | ||
let(:listview_title_selector) {' > span.title'} | ||
let(:listview_filename_selector) {' > span.preview'} |
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.
this changeset is a great example of why aligning things isn't ideal 😄
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.
Yeah, good to know :)
@parndt Are you ok to merge this PR ? |
I note there are no tests specific to this feature? |
Is someone could help me to test this PR ? |
180446e
to
a531e3b
Compare
@anitagraham do you have any ideas? I know you've been working on images specs lately. |
Yes, having a look at how the translations for page title are tested. |
Thanks @anitagraham, it's really appreciated! |
So, to confirm: is the point of this feature so that the user can avoid uploading an image per locale? |
Yes. |
i will update this really soon PR :) |
a531e3b
to
54f4018
Compare
54f4018
to
b22eab5
Compare
It's ready @parndt :) |
end | ||
|
||
def self.down | ||
Refinery::Image.drop_translation_table! migrate_data: true |
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.
@bricesanchez sorry, I missed the migration problems before 😦 |
b22eab5
to
73cbef5
Compare
@parndt no problems, is it ok like that now? |
OK, I'm happy with this. |
Add ability to translate image_title and image_alt fields
No description provided.