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

704: Add glossary tooltips after editing a row. #705

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@toolstack
Contributor

toolstack commented Apr 27, 2017

Resolves #704.

@toolstack toolstack requested a review from ocean90 Apr 27, 2017

@toolstack toolstack added this to the 2.4 milestone Apr 27, 2017

@@ -315,6 +317,12 @@ public function translations_post( $project_path, $locale_slug, $translation_set
$translations = GP::$translation->for_translation( $project, $translation_set, 'no-limit', array('translation_id' => $translation->id), array() );
if ( ! empty( $translations ) ) {
if ( $glossary ) {
require_once GP_TMPL_PATH . 'helper-functions.php';

This comment has been minimized.

@ocean90

ocean90 Apr 30, 2017

Member

Since map_glossary_entries_to_translations_originals() is currently a template function there's no guarantee that the function exists. Maybe it shouldn't be a template function? Would gp-includes/template.php be a better place?

@ocean90

ocean90 Apr 30, 2017

Member

Since map_glossary_entries_to_translations_originals() is currently a template function there's no guarantee that the function exists. Maybe it shouldn't be a template function? Would gp-includes/template.php be a better place?

This comment has been minimized.

@toolstack

toolstack May 1, 2017

Contributor

I think it should be a template function as it's creating html for the front end to use. A custom template might want to change it from a tooltip to something else and need different markup to do it.

I'd say if someone has created a new theme and template functions that it would have to support all functions in the helper file to properly support outputting the glossary markup.

The other way to do this, which might be logically cleaner, would be to create a new template "translation-row-ajax.php" which would call map_glossary_entries_to_translations_originals() from inside of it to keep all of the template logic inside the template directory.

@toolstack

toolstack May 1, 2017

Contributor

I think it should be a template function as it's creating html for the front end to use. A custom template might want to change it from a tooltip to something else and need different markup to do it.

I'd say if someone has created a new theme and template functions that it would have to support all functions in the helper file to properly support outputting the glossary markup.

The other way to do this, which might be logically cleaner, would be to create a new template "translation-row-ajax.php" which would call map_glossary_entries_to_translations_originals() from inside of it to keep all of the template logic inside the template directory.

@ocean90

This comment has been minimized.

Show comment
Hide comment
@ocean90

ocean90 May 30, 2017

Member

Closing in favor of #716.

Member

ocean90 commented May 30, 2017

Closing in favor of #716.

@ocean90 ocean90 closed this May 30, 2017

@toolstack toolstack deleted the 704-missing-glossary-tooltip-after-edit branch Jun 24, 2017

@toolstack toolstack modified the milestones: 3.0, 2.4 Aug 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment