Closed Bug 1209041 Opened 9 years ago Closed 9 years ago

(Gaia RTL 2.5) RTL support for <gaia-header>

Categories

(Firefox OS Graveyard :: Gaia, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(feature-b2g:2.5+)

RESOLVED FIXED
feature-b2g 2.5+

People

(Reporter: kaze, Assigned: kaze)

References

Details

Attachments

(5 files)

According to the 2.5 RTL specs, the header bars must now be mirrored in RTL.
Attached file RTL support
Assignee: nobody → kaze
Status: NEW → ASSIGNED
Attachment #8666698 - Flags: review?(wilsonpage)
Comment on attachment 8666698 [details] [review]
RTL support

Great stuff! Thanks mate!
Attachment #8666698 - Flags: review?(wilsonpage) → review+
https://github.com/gaia-components/gaia-header/commit/a6c2e45335ff8613bb5b3b429ee23c6c80b54b05
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Thanks Wilson!

Should I open another bug to bump the version number, so we can include the latest version in Gaia? Or do you handle this yourself?
Reopening to track v0.9.0 landing in Gaia repo.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I think this is the best we can do to work around bug 1100912: when the `dirObserver` property is true, automatically set a `dir` attribute to all shadow DOM children when the component is created and when the document direction is changed.

I’d prefer implementing this in <gaia-component> rather than <gaia-header> to make it easier to RTL-ize other Gaia components.
Attachment #8668439 - Flags: review?(wilsonpage)
I forgot to say: with this patch, we can set a `dir="ltr"` attribute to any Gaia component (that depend on <gaia-component>). Thus, we could deploy the new <gaia-header> in /shared/elements — we’ll just have to add a `dir="ltr"` attribute in applications that haven’t been refactored yet.
Comment on attachment 8668439 [details] [review]
<gaia-component> workaround for bug 1100912

Couple of NITs but r+ once they're done :) Nice one!
Attachment #8668439 - Flags: review?(wilsonpage) → review+
Attached file <gaia-header> update
With this patch + the one on <gaia-component>, the `dir` attribute on a <gaia-header> component will work as expected.
Attachment #8668876 - Flags: review?(wilsonpage)
Comment on attachment 8668904 [details] [review]
[gaia] fabi1cazenave:gaia-header-update-bug1209041 > mozilla-b2g:master

This is the last step: adding `dir="ltr"` to all <gaia-header> in gaia apps to preserve the navigation consistency. Exceptions:
 • System, because it’s already been refactored
 • Camera and Music, because they use their own, older version of gaia-header
Attachment #8668904 - Flags: review?(wilsonpage)
Comment on attachment 8668904 [details] [review]
[gaia] fabi1cazenave:gaia-header-update-bug1209041 > mozilla-b2g:master

julien: Could you give this patch a sanity check. GaiaHeader hasn't been updated in Gaia since v0.8.0 (not v0.9.2).

This comes with some changes:

- min-font-size for titles reduced to 16px
- improved rtl helpers (gaia-component)
- improved extend features (gaia-component)
Attachment #8668904 - Flags: feedback?(felash)
Julien, note that applying this patch should be (almost) transparent for all Gaia apps. To clarify:

 • all apps (except Camera, Music and System) should remain unchanged until we remove the `dir="ltr"` attributes from the <gaia-header> elements;

 • the only visible change will be in the /shared/elements/gaia-header/index.html demo page: the header bar gets mirrored in RTL.
Comment on attachment 8668904 [details] [review]
[gaia] fabi1cazenave:gaia-header-update-bug1209041 > mozilla-b2g:master

This patch will affect all apps. I'm pretty confident it will be fine as we have no massive changes. r+ once julienw has given it a quick look too.
Attachment #8668904 - Flags: review?(wilsonpage) → review+
feature-b2g: --- → 2.5+
Priority: -- → P1
Following our discussion with Julien, here’s a patch on <gaia-header> to add an `ignoreDir` attribute to force the former LTR behavior of the component — especially regarding for title direction.

If this is OK, for all apps that are not ready yet for RTL navigation, we could use:
    <gaia-header ignoreDir>

instead of:
    <gaia-header dir="ltr">

Wilson, WDYT? If this lands in gaia-components (with a version bump) I’ll update the Gaia patch accordingly.
Attachment #8670819 - Flags: review?(wilsonpage)
But the good thing is: this is working !
Thanks for the early feedback Julien! Your comments have been addressed, PR updated.
I still have a very small concern but that will have no importance in the real use cases we're interested in. So feel free to disregard it.

I tried it and it seems to work fine in the SMS app in a lot of situations.
Attachment #8668904 - Flags: feedback?(felash)
Comment on attachment 8670819 [details] [review]
<gaia-header ignoreDir>

There seems to be an issue with some of the built files. I suggest making usre all bower_components are up to date and repo is rebased before `npm run build` again.

Code changes look good though :)
Attachment #8670819 - Flags: review?(wilsonpage) → review+
Rebased and rebuilt, ready to merge.
Blocks: 1209415
Merged on master: https://github.com/mozilla-b2g/gaia/commit/8331763

Thanks Wilson, thanks Julien!
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: