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

feat(module:all): Add RTL support to all ng-zorro-antd #4703

Merged
merged 177 commits into from
Dec 15, 2020

Conversation

saeedrahimi
Copy link
Contributor

@saeedrahimi saeedrahimi commented Jan 20, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #4704 #1762

What is the new behavior?

All components will be displayed correctly in RTL direction.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@saeedrahimi saeedrahimi changed the title [WIP] Add RTL support to all ng-zorro-antd feat(module:table): Add RTL support to all ng-zorro-antd Jan 20, 2020
@codecov
Copy link

codecov bot commented Jan 20, 2020

Codecov Report

Merging #4703 (bfd485f) into master (8b92cac) will decrease coverage by 0.01%.
The diff coverage is 91.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4703      +/-   ##
==========================================
- Coverage   89.84%   89.82%   -0.02%     
==========================================
  Files         468      469       +1     
  Lines       14262    14948     +686     
  Branches     2242     2273      +31     
==========================================
+ Hits        12813    13427     +614     
- Misses        894      935      +41     
- Partials      555      586      +31     
Impacted Files Coverage Δ
components/carousel/typings.ts 100.00% <ø> (ø)
components/checkbox/checkbox-wrapper.component.ts 100.00% <ø> (ø)
components/comment/demo/basic.ts 100.00% <ø> (ø)
components/core/config/config.ts 100.00% <ø> (ø)
components/tabs/tabs-ink-bar.directive.ts 83.87% <ø> (ø)
components/date-picker/picker.component.ts 87.42% <37.50%> (-2.13%) ⬇️
components/upload/upload.component.ts 94.35% <44.44%> (-3.94%) ⬇️
components/tree/tree.component.ts 92.65% <50.00%> (-1.50%) ⬇️
components/typography/typography.component.ts 95.83% <57.14%> (-2.00%) ⬇️
components/modal/modal-container.ts 91.87% <62.50%> (-1.28%) ⬇️
... and 82 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b92cac...bfd485f. Read the comment docs.

@saeedrahimi saeedrahimi mentioned this pull request Jan 20, 2020
62 tasks
@saeedrahimi saeedrahimi changed the title feat(module:table): Add RTL support to all ng-zorro-antd feat(module:all): Add RTL support to all ng-zorro-antd Jan 21, 2020
@vthinkxie
Copy link
Member

Hi @saeedrahimi
thanks for your pull request!
most components have been refactored now, it is safe to rebase and finish RTL support now :)

@vthinkxie
Copy link
Member

vthinkxie commented Apr 20, 2020

Hi @saeedrahimi
how is everything going?
we have released 9.0 version and make RTL to the 9.1.0 milestone
if you need any help, please let us know, thanks a lot for your contributions!

@hdm91
Copy link
Contributor

hdm91 commented May 5, 2020

Hi @vthinkxie, @saeedrahimi
I can help you to complete RTL support from two to four weeks, if there isn't any problem tell me to start the process.
thanks

@@ -0,0 +1 @@
node-options=--max-old-space-size=14000
Copy link
Member

Choose a reason for hiding this comment

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

This is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️ No, it's for my local development


ngOnInit(): void {
this.dir = this.directionality.value;
this.directionality.change.subscribe((direction: Direction) => {
Copy link
Member

Choose a reason for hiding this comment

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

need pipe(takeUntil(this.destroy$)) here

@hsuanxyz
Copy link
Member

@saeedrahimi @HDaghash @hdm91

Thank for your contribution, it looks sufficiently great now. We would like to mention your contribution in the release log, so could you please tell us your twitter IDs?

@@ -65,6 +66,7 @@ export type NzDatePickerSizeType = 'large' | 'default' | 'small';
[placeholder]="nzPlaceHolder"
style="display: inherit; align-items: center; width: 100%;"
[dropdownClassName]="nzDropdownClassName"
Copy link
Member

@wenqi73 wenqi73 Dec 15, 2020

Choose a reason for hiding this comment

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

add [dir]="dir"

Comment on lines +116 to +117
[style.left]="datePickerService?.arrowLeft"
[style.right]="datePickerService?.arrowRight"
Copy link
Member

@wenqi73 wenqi73 Dec 15, 2020

Choose a reason for hiding this comment

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

Suggested change
[style.left]="datePickerService?.arrowLeft"
[style.right]="datePickerService?.arrowRight"
[ngStyle]="activeBarStyle"

Comment on lines +295 to +305

if (this.dir === 'rtl') {
this.datePickerService.arrowRight =
this.datePickerService.activeInput === 'right' ? this.inputWidth + this.separatorElement?.nativeElement.offsetWidth || 0 : 0;
this.datePickerService.arrowLeft = 'auto';
} else {
this.datePickerService.arrowLeft =
this.datePickerService.activeInput === 'left' ? 0 : this.inputWidth + this.separatorElement?.nativeElement.offsetWidth || 0;
this.datePickerService.arrowRight = 'auto';
}

Copy link
Member

@wenqi73 wenqi73 Dec 15, 2020

Choose a reason for hiding this comment

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

Suggested change
if (this.dir === 'rtl') {
this.datePickerService.arrowRight =
this.datePickerService.activeInput === 'right' ? this.inputWidth + this.separatorElement?.nativeElement.offsetWidth || 0 : 0;
this.datePickerService.arrowLeft = 'auto';
} else {
this.datePickerService.arrowLeft =
this.datePickerService.activeInput === 'left' ? 0 : this.inputWidth + this.separatorElement?.nativeElement.offsetWidth || 0;
this.datePickerService.arrowRight = 'auto';
}
const baseStyle = { position: 'absolute', width: `${this.inputWidth}px` };
this.datePickerService.arrowLeft = this.datePickerService.activeInput === 'left' ? 0 : this.inputWidth + this.separatorElement?.nativeElement.offsetWidth || 0;
if (this.dir === 'rtl') {
this.activeBarStyle = { ...baseStyle, right: `${this.datePickerService.arrowLeft}px` };
} else {
this.activeBarStyle = { ...baseStyle, left: `${this.datePickerService.arrowLeft}px` };
}

also need to define activeBarStyle.

Comment on lines +17 to +18
arrowLeft: string = 'auto';
arrowRight: string = 'auto';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
arrowLeft: string = 'auto';
arrowRight: string = 'auto';
arrowLeft: number = 0;

Copy link
Member

@wenqi73 wenqi73 left a comment

Choose a reason for hiding this comment

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

the active bar in range picker cannot work, need some changes.

@saeedrahimi
Copy link
Contributor Author

saeedrahimi commented Dec 15, 2020

@saeedrahimi @HDaghash @hdm91

Thank for your contribution, it looks sufficiently great now. We would like to mention your contribution in the release log, so could you please tell us your twitter IDs?

I'm glad I could help.
twitter ID :@Saeedr_ir

@HDaghash
Copy link
Contributor

@saeedrahimi @HDaghash @hdm91

Thank for your contribution, it looks sufficiently great now. We would like to mention your contribution in the release log, so could you please tell us your twitter IDs?

Thank you guys for making this happen
Glad to hear
Twitter :@HDagash

@saeedrahimi
Copy link
Contributor Author

@saeedrahimi @HDaghash @hdm91

Thank for your contribution, it looks sufficiently great now. We would like to mention your contribution in the release log, so could you please tell us your twitter IDs?

Someone missed to mention.
I want to thank @hmdnikoo for all his contributions. he kindly changed most of the components to fix a bug on RTL change.

@yangjunhan
Copy link
Contributor

@saeedrahimi @HDaghash @hdm91
Thank for your contribution, it looks sufficiently great now. We would like to mention your contribution in the release log, so could you please tell us your twitter IDs?

Someone missed to mention.
I want to thank @hmdnikoo for all his contributions. he kindly changed most of the components to fix a bug on RTL change.

Certainly. We value all your contribution very much. Please also provide his Twitter id. By the way, do you have @hdm91 's Twitter id?

@hdm91
Copy link
Contributor

hdm91 commented Dec 15, 2020

@saeedrahimi @HDaghash @hdm91
Thank for your contribution, it looks sufficiently great now. We would like to mention your contribution in the release log, so could you please tell us your twitter IDs?

Someone missed to mention.
I want to thank @hmdnikoo for all his contributions. he kindly changed most of the components to fix a bug on RTL change.

Certainly. We value all your contribution very much. Please also provide his Twitter id. By the way, do you have @hdm91 's Twitter id?

Hi guys , thanks for your consideration and helping to make RTL feature on great ng zorro , I wish I would have enough time to help you in future
my twitter ID :@hdm9170

@hsuanxyz hsuanxyz merged commit 860dfed into NG-ZORRO:master Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants