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

RESOLVED FIXED

Status

Firefox OS
Gaia
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kaze, Assigned: kaze)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.5+)

Details

Attachments

(5 attachments)

(Assignee)

Description

3 years ago
According to the 2.5 RTL specs, the header bars must now be mirrored in RTL.
(Assignee)

Comment 1

3 years ago
Created attachment 8666698 [details] [review]
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 4

3 years ago
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 → ---
(Assignee)

Comment 7

3 years ago
Created attachment 8668439 [details] [review]
<gaia-component> workaround for bug 1100912

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)
(Assignee)

Comment 8

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1202354
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+
(Assignee)

Comment 10

3 years ago
Created attachment 8668876 [details] [review]
<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)
Created attachment 8668904 [details] [review]
[gaia] fabi1cazenave:gaia-header-update-bug1209041 > mozilla-b2g:master
(Assignee)

Comment 13

3 years ago
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)
(Assignee)

Comment 15

3 years ago
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+

Updated

3 years ago
feature-b2g: --- → 2.5+
Priority: -- → P1
(Assignee)

Comment 17

3 years ago
Created attachment 8670819 [details] [review]
<gaia-header ignoreDir>

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 !
(Assignee)

Comment 20

3 years ago
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+
(Assignee)

Comment 23

3 years ago
Rebased and rebuilt, ready to merge.
(Assignee)

Updated

3 years ago
(Assignee)

Updated

3 years ago
Blocks: 1209415
(Assignee)

Comment 24

3 years ago
Merged on master: https://github.com/mozilla-b2g/gaia/commit/8331763

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