Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix(rtl): support applying dir="rtl" on DOM elements other than the body #11579

Merged
merged 1 commit into from
Jul 11, 2019

Conversation

marosoft
Copy link
Contributor

@marosoft marosoft commented Jan 7, 2019

Create a single function to check text direction based on dir attribute on DOM element or document.

Fixes #11016 Fixes #9754

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[x] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

A whole document is expected to be set as RTL even when a dir attribute is set properly on an individual element.
Issue Number: #11016, #9754

What is the new behavior?

It is possible to use dir attribute on an element to override a document's dir value.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jan 7, 2019
@Splaktar Splaktar self-requested a review January 8, 2019 22:20
@Splaktar Splaktar self-assigned this Jan 8, 2019
@Splaktar Splaktar added i18n: bi-directional This issue is related to internationalization and right to left languages P4: minor Minor issues. May not be fixed without community contributions. labels Jan 22, 2019
@Splaktar Splaktar added this to the 1.1.13 milestone Jan 22, 2019
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have a few minor requests for changes and a question.

It would be good to add some documentation for this feature somewhere. Perhaps in a Localization Guide in docs/content/localization using Markdown?

src/components/virtualRepeat/virtual-repeater.js Outdated Show resolved Hide resolved
src/core/util/util.js Show resolved Hide resolved
src/core/util/util.js Outdated Show resolved Hide resolved
src/core/util/util.js Show resolved Hide resolved
@Splaktar Splaktar added needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs P3: important Important issues that really should be fixed when possible. type: enhancement and removed P4: minor Minor issues. May not be fixed without community contributions. labels Jan 22, 2019
@Splaktar Splaktar modified the milestones: 1.1.13, 1.1.14 Feb 10, 2019
@Splaktar Splaktar modified the milestones: 1.1.14, 1.1.15 Mar 14, 2019
@Splaktar Splaktar modified the milestones: 1.1.15, 1.1.16, 1.1.18, 1.1.19 Mar 29, 2019
@Splaktar
Copy link
Member

@marosoft can you please rebase this PR and address the review comments when you get a chance?

@Splaktar Splaktar modified the milestones: 1.1.19, 1.1.20 May 30, 2019
@marosoft
Copy link
Contributor Author

@Splaktar I noticed that bidi is mentioned in the Migration Guide.
In the util.js file there are three methods related: isRtl, bidi and bidiProperty.

Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested some improvements to the JSDoc.

Thanks for the notes for the Migration guide. Would you be interested in adding those to the guide in another PR after this one gets merged?

src/core/util/util.js Outdated Show resolved Hide resolved
src/core/util/util.js Outdated Show resolved Hide resolved
@Splaktar Splaktar removed the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label May 30, 2019
Create a single function to check text direction based on dir attribute on DOM element or document.

Fixes angular#11016 Fixes angular#9754
@marosoft
Copy link
Contributor Author

marosoft commented May 31, 2019

I suggested some improvements to the JSDoc.

Thanks for the notes for the Migration guide. Would you be interested in adding those to the guide in another PR after this one gets merged?

@Splaktar I made the suggested changes, thanks for those.
I can add the notes to the guide. Do I need to create an issue for that or can I reference any existing one?

@Splaktar
Copy link
Member

No issue needed. Please just reference this PR. Thank you!

Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would really like some basic unit tests to verify this functionality if possible, but I don't want to block getting this in for many weeks. Please let me know if you can add a couple basic tests soon, otherwise, can you please open an issue to add tests for this?

Also can you please open an issue to create a new Localization Guide in docs/content/localization (using Markdown)?

Thank you for this important work!

@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels May 31, 2019
@Splaktar Splaktar added the pr: lgtm This PR has been approved by the reviewer label May 31, 2019
@marosoft
Copy link
Contributor Author

@Splaktar I found this issue which I might use as a reference for the documentation: #8615

I might find some time over the weekend to add some basic unit tests. Do you want me to add those for each component impacted by the change together with the changes I made to util.js ?

@Splaktar
Copy link
Member

Ideally, it would be nice to see a couple tests for each affected component (in their spec.js files) and then some more general tests in util.spec.js. It would help us make sure that this functionality kept working over the long term.

Affected components

  • menu
  • panel
  • slider
  • sticky
  • tabs
  • grid-list
  • virtual-repeat

Good catch on that RTL docs issue!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ i18n: bi-directional This issue is related to internationalization and right to left languages P3: important Important issues that really should be fixed when possible. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review type: enhancement
Projects
None yet
7 participants