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

Update RTL examples #33097

Merged
merged 11 commits into from Apr 27, 2021
Merged

Update RTL examples #33097

merged 11 commits into from Apr 27, 2021

Conversation

aqeelat
Copy link
Contributor

@aqeelat aqeelat commented Feb 15, 2021

Hi,

The current RTL examples have many inconsistencies and mistranslations. I'm taking a deeper look and translating them better.
Don't merge yet.

@XhmikosR XhmikosR changed the title Draft: updated RTL examples Update RTL examples Feb 15, 2021
@ffoodd ffoodd mentioned this pull request Feb 16, 2021
20 tasks
@ffoodd
Copy link
Member

ffoodd commented Feb 16, 2021

Thanks a lot for tackling this, this is highly appreciated!
Please tell us if we can help or if you have any questions.

@aqeelat
Copy link
Contributor Author

aqeelat commented Feb 18, 2021

Do I really have to rebase? it's really creating a bunch of unneeded commits.

@ffoodd
Copy link
Member

ffoodd commented Feb 19, 2021

It shouldn't, but if your PR is ready we'll rebase it by ourselves if needed.

@aqeelat
Copy link
Contributor Author

aqeelat commented Feb 19, 2021

I think I'm done with all examples, except for the cheatsheet, which would need to be looked at by a fresh pair of eyes. Pinging @aldilaff and @Nuhasami
Should I provide RTL versions of other examples? If so, which ones should I prioritize? I would probably add them as separate PRs though.

@@ -195,7 +198,7 @@ <h4 class="fst-italic">في مكان آخر</h4>
</main><!-- /.container -->

<footer class="blog-footer">
<p>تم تصميم نموذج المدونة لـ <a href="https://getbootstrap.com/"> Bootstrap </a> بواسطة <a href="https://twitter.com/mdo">mdo</a>.</p>
<p>تم تصميم نموذج المدونة لـ <a href="https://getbootstrap.com/">Bootstrap</a> بواسطة <a href="https://twitter.com/mdo"><bdi lang="en" dir="ltr">@mdo</bdi></a>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Snapshoting this in my mind for our future bi-di example templates ❤️

@ffoodd
Copy link
Member

ffoodd commented Feb 19, 2021

@aqeelat We prioritized some examples in our first implementation —based on how much they'd change in RTL— but feel free to add some of the other ones if you feel it makes sense.

In #32330 I added in the "Need help" part a topic regarding bi-directional templates. We don't have any of them and I'm not sure how to tackle this, so if you have any insights (or even bi-di contents in the wild) that could help a lot. We really need some examples in order to document how-to for bi-di layouts since it'd been asked a few times already.

This PR is pushing the overall quality of our RTL support a lot, that's unvaluable so thanks a lot!

@aqeelat
Copy link
Contributor Author

aqeelat commented Feb 19, 2021

@ffoodd I'm glad I was able to help. As for bi-di, I already used it extensively in the cheatsheet. Check out the tables there. However, I'll try to come up with a dedicated example.

@aqeelat
Copy link
Contributor Author

aqeelat commented Feb 21, 2021

Reminder: we would need to recreate the screenshots before the merge.

@ffoodd
Copy link
Member

ffoodd commented Feb 23, 2021

Would it make any sense to use eastern Arabic numerals in examples, @aqeelat?
This is kindly asked by @Mrahmani71 in #33170: comment.

This is the exact kind of details we need help for, since we don't have any Arabic speaker in the team. Your efforts are highly appreciated!

@aqeelat
Copy link
Contributor Author

aqeelat commented Feb 27, 2021

@ffoodd Many Arabic speakers use Hindu numerals. I guess it makes sense to use them in the examples. I'll update the examples.

@ffoodd
Copy link
Member

ffoodd commented Apr 15, 2021

@aqeelat Friendly ping here, do you think it's safe to merge or do you see anything else to do—apart from new screenshots, which I'll handle?

@aqeelat
Copy link
Contributor Author

aqeelat commented Apr 25, 2021

I'm sorry I was crazy busy in the past couple of weeks. I'll do a final review and update you within 24 hours.

@ffoodd
Copy link
Member

ffoodd commented Apr 25, 2021

No worry nor hurry, just wanted to check back if we didn't forget this 🙂

@aqeelat
Copy link
Contributor Author

aqeelat commented Apr 27, 2021

Looks good to me. I think it's ready to be merged.

@aqeelat aqeelat marked this pull request as ready for review April 27, 2021 07:22
@aqeelat aqeelat requested a review from a team as a code owner April 27, 2021 07:22
@aqeelat
Copy link
Contributor Author

aqeelat commented Apr 27, 2021

And for the Hindu-Arabic numerals comment that was mentioned in another PR of mine, I'm a strong believer that Arabic text should have Arabic numerals, and therefore we should not replace them in these examples.

@ffoodd
Copy link
Member

ffoodd commented Apr 27, 2021

Fair enough, thanks a lot!

@XhmikosR XhmikosR added this to Inbox in v5.0.0 via automation Apr 27, 2021
@XhmikosR XhmikosR merged commit 3bcb9e7 into twbs:main Apr 27, 2021
v5.0.0 automation moved this from Inbox to Done Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

Text placement is not in the correct place in the example checkout-rtl
3 participants