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.

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

org settings: De-duplicate template content for checkboxes. #8909

Merged

Conversation

shubhamdhama
Copy link
Member

  • There is no UI change.
  • Not all labels are moved to JS code for translation, only those whose templates are deduplicated are moved in admin.js
  • Some checkboxes can fit in the settings_checkbox.handlebars by using end_content but because of translation strings like title makes it difficult to do so, e.g. id_realm_allow_message_deleting.

(It is a preliminary change for deduplication of org settings template.)
This is done because of some settings like organization-settings
has ids which match the pattern of having a prefix `id_`
before the property name.

For those settings which don't have any prefix, there will be no
effect.
Here obsolete `t` in the label is removed since we do
label translations in JS files.
(It is a preliminary change for deduplication of org settings template.)
This adds org settings labels as a context to admin templates so that
they can be used as a context variables in admin templates.
The reason we did this in JS code because of translation issue when
passed (as a context in `partial` handlebars helper) directly within
template.
@timabbott
Copy link
Sponsor Member

Looks great, merged, thanks @shubhamdhama!

I think we probably will want to do a further refactor to get rid of the id_ prefix for these, but I think it was the right call to first do the direct translation.

@timabbott timabbott closed this Apr 1, 2018
@timabbott timabbott merged commit 85077fb into zulip:master Apr 1, 2018
@shubhamdhama
Copy link
Member Author

shubhamdhama commented Apr 1, 2018

I can include this as a followup if we really want to do this.

@timabbott
Copy link
Sponsor Member

Yeah, I think it's a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants