Skip to content

is_rtl considered harmful #216

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

Closed
simonwheatley opened this issue Mar 2, 2015 · 13 comments
Closed

is_rtl considered harmful #216

simonwheatley opened this issue Mar 2, 2015 · 13 comments
Assignees
Labels
Milestone

Comments

@simonwheatley
Copy link
Contributor

Currently our is_rtl function parses (eugh) any locale file and checks for RTL. Instead we should just hardcode the RTL languages into an array (really, how often is a new one going to come along), and allow the dev to filter it so they can add any special case they may require.

@simonwheatley simonwheatley added this to the Code Review milestone Mar 2, 2015
simonwheatley referenced this issue Mar 2, 2015
@markoheijnen
Copy link
Contributor

Why do we have is_rtl? Is this value needed or can it be dealt with on runtime?

@simonwheatley
Copy link
Contributor Author

See this issue suggesting using Glotpress' locale class, which @hewsut has also just suggested: #76

Also relevant, this PR from December to add language install capabilities: #201

@simonwheatley
Copy link
Contributor Author

Why do we have is_rtl? Is this value needed or can it be dealt with on runtime?

I think this is just so we can have the information in the available languages screen. At runtime it's dealt with by loading the language locale information, not doing some crazy parsing of PHP to avoid loading it. 😄

@markoheijnen
Copy link
Contributor

I thought so, so if it's only for showing information then why bot removing it? I assume the user would know if the language is RTL or not 😉

@simonwheatley
Copy link
Contributor Author

Removing it is another solution. Thinking.

@simonwheatley simonwheatley self-assigned this Jul 7, 2015
@simonwheatley
Copy link
Contributor Author

Hard coding a list of languages in Babble in some way, seems best.

@nb
Copy link
Member

nb commented Jul 9, 2015

@johnbillion
Copy link
Contributor

I'd completely forgotten that core uses an ltr string for specifying text direction. Maybe Babble should switch to that.

I'll check the RTL languages for core and see if they all use it.

@johnbillion johnbillion reopened this Jul 9, 2015
simonwheatley added a commit that referenced this issue Jul 10, 2015
* develop:
  Remove the reliance on `$lang.php` and use our static list of RTL languages. See #216.
  Introduce a static list of RTL languages from GlotPress and Wikipedia.
  This query doesn't contribute to the pre-caching of the subsequent `get_post()` calls, because they've already been fetched by `bbl_get_default_lang_post()` above. Fixes #251.
  If a term doesn't have a transid, allow it to fall through so one is created rather than throwing an exception.
  Remove the unused term resyncing functionality which hasn't been used since #106.
  Validate the content lang and interface lang before setting them. See #227.
  Remove error suppression on the term editing screen. See #223.
  Remove error suppression when saving language options or viewing the language options admin screen. See #223.
  Remove unnecessary theme functionality from the base `Babble_Plugin` class. See #223.
  Avoid throwing exceptions in `Babble_Taxonomies::get_transid()` and `Babble_Taxonomies::set_transid()`. See #218.
  Make the `$name` parameter in `Babble_Plugin::setup()` a required parameter, avoid throwing an exception. See #218.
  Avoid throwing an exception in `is_public_lang()`. Now accepts a language object or a language code string. See #218.

Conflicts:
	class-taxonomy.php
@johnbillion
Copy link
Contributor

Closing in favour of #216

@willmot
Copy link
Contributor

willmot commented Jul 10, 2015

@johnbillion you closed this in favour of itself?

@johnbillion
Copy link
Contributor

Whoops, #298

@simonwheatley
Copy link
Contributor Author

@johnbillion you closed this in favour of itself?

Where's the ‘like’ button on this thing?

simonwheatley added a commit that referenced this issue Jul 13, 2015
* develop: (47 commits)
  Correct a test name.
  Travis config: Added `sudo: false` to use modern infrastructure
  Remove the reliance on `$lang.php` and use our static list of RTL languages. See #216.
  Introduce a static list of RTL languages from GlotPress and Wikipedia.
  Tests for post translations. See #236.
  Remove a bunch of unused code.
  Remove empty post IDs from the `$post_ids` array.
  Remove unnecessary use of global API functions, use current object methods instead.
  `Babble_Post_Public::get_post_translations()` must always return an array of `WP_Post` objects.
  Retain the deletion of the old post translation cache keys so they don't stay in memory in perpetuity.
  This query doesn't contribute to the pre-caching of the subsequent `get_post()` calls, because they've already been fetched by `bbl_get_default_lang_post()` above. Fixes #251.
  If a term doesn't have a transid, allow it to fall through so one is created rather than throwing an exception.
  Remove the unused term resyncing functionality which hasn't been used since #106.
  Correctly exit with non-zero if there are syntax errors when testing on Travis. See #286.
  Ignore Composer lock and vendor directory
  Validate the content lang and interface lang before setting them. See #227.
  Remove error suppression on the term editing screen. See #223.
  Remove error suppression when saving language options or viewing the language options admin screen. See #223.
  Remove unnecessary theme functionality from the base `Babble_Plugin` class. See #223.
  Avoid throwing exceptions in `Babble_Taxonomies::get_transid()` and `Babble_Taxonomies::set_transid()`. See #218.
  ...
simonwheatley added a commit that referenced this issue Aug 5, 2015
* develop: (127 commits)
  Fix merge issues in `class-taxonomy.php`
  More test coverage for post and term translations.
  Correctly flush term translation caches.
  Switch to using transids for term translation cache keys.
  Travis config: Travis config: Added `sudo: false` to use modern infrastructure
  Correct a test name.
  Travis config: Added `sudo: false` to use modern infrastructure
  Babble_Locale: Use an exploit priority, rather than null
  Babble_Taxonomies: Change from add_action method to core function
  Babble_Languages: Change from add_action method to core function
  Remove the reliance on `$lang.php` and use our static list of RTL languages. See #216.
  Introduce a static list of RTL languages from GlotPress and Wikipedia.
  Add tests to check post and term translations after adding a new translation.
  Use database query numbers as tests to ensure our post and term translation caching is working as expected.
  Add caching to `Babble_Taxonomies::get_term_translations()`. Avoids a call to `get_objects_in_term()`. See #246.
  Tests for term translations, and change `Babble_Taxonomies::get_term_translations()` so it returns real term objects.
  Tests for post translations. See #236.
  Remove a bunch of unused code.
  Remove empty post IDs from the `$post_ids` array.
  Remove unnecessary use of global API functions, use current object methods instead.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants