-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Bidirectional improvements for core classes #9148
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
base: master
Are you sure you want to change the base?
Conversation
Confirmed: Leilei332 has already signed the Contributor License Agreement (see contributing.md) |
✅ Deploy Preview for tiddlywiki-previews ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Thank you @Leilei332. @saqimtiaz could we add this to the v5.4.0 board? |
@Leilei332 ... How can we test these changes. |
It is not possible to test it in netlify preview, because there is a bug with You have to checkout the PR and serve the full edition, switch to a rtl language to test it. |
What is the full edition, tw5-com edition is the largest edition. @kookma Might be interested. |
https://tiddlywiki.com/editions/full/ locally it needs to be started with node.js dev environment
|
@Jermolene -- There definitely are some improvements -- So we should merge improvements one by one. @kookma -- Do you have an issue, with some screenshots, what's still wrong in v5.3.6 -- IMO with screenshots it will be easier for us to fix the bugs. v5.3.6 More -> Tools sidebar ![]() this PR ![]() |
@Leilei332 .. If you would add some screenshots to your PR it would make it easier to test. I use https://www.irfanview.com/ to make my GIF or PNG screenshots. With F12 I can add the arrows and some text if needed. |
Thanks @Leilei332 @kookma @pmario. I think these are very important improvements. Our goal should be for RTL support to be on an equal footing with our LTR support, to reflect our ambitions of universality. |
Can I use this for testing? |
@kookma I believe this is the correct link for testing this PR: https://deploy-preview-9148--tiddlywiki-previews.netlify.app/editions/full/ |
I will check! but right now the in screenshot you shared, the PR corrected the space issue. |
Just tested on https://deploy-preview-9148--tiddlywiki-previews.netlify.app/editions/full/ ![]() In the image above, one dropdown shows the arrow on the right, while another has it on the left. The spacing between the button and text is incorrect, and the text overlaps with the button. There are recommendations and standards for RTL. I'm not sure if this is the right place to discuss it, but I think it's a good idea to follow W3C recommendations for RTL design in TiddlyWiki, especially if there's a decision to enhance TiddlyWiki's bidi support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kookma -- Do you have an issue, with some screenshots, what's still wrong in v5.3.6 -- IMO with screenshots it will be easier for us to fix the bugs.
Just tested on https://deploy-preview-9148--tiddlywiki-previews.netlify.app/editions/full/
See the $:/ControlPanel has issues.![]()
In the image above, one dropdown shows the arrow on the right, while another has it on the left. The spacing between the button and text is incorrect, and the text overlaps with the button.
There are recommendations and standards for RTL. I'm not sure if this is the right place to discuss it, but I think it's a good idea to follow W3C recommendations for RTL design in TiddlyWiki, especially if there's a decision to enhance TiddlyWiki's bidi support.
This problem cannot be solved with only CSS, because the text direction is hardcoded in the wikitext source code, which adds align="left"
in HTML. We have to remove the align in the source code and use CSS to align it instead.
The problem of direction was fixed.
BTW, the align
attribute is marked as deprecated in MDN, we may switch to use the style
attribute in the parser.
📊 Build Size Comparison:
|
Branch | Size |
---|---|
Base (master) | 2537.5 KB |
PR | 2540.5 KB |
Diff: ⬆️ Increase: +2.9 KB
@Leilei332 -- As @kookma mentioned at: #9148 (comment) the right sidebar can stay on the right. @kookma -- You wrote, that you also did some adjustments. Can you show them here, so Leilei can include them? |
Well I use #7557 the onging work by @Jermolene
I think this is useless in the context of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like using CSS Logical properties and values not only simplifies code, but also supports a wider range of browsers (since the :dir()
pseudo class requires browsers released after 2023). Sadly, some CSS border logical properties requires browsers released after 2019, which is in an awkward situation.
Thanks @Leilei332 I wasn't aware of this module, but agree that this approach makes a lot of sense.
Would it be practical to provide fallbacks for older browsers? |
@Leilei332 |
IMO currently the best practice to support older browsers is to use |
Looks like I got it wrong here. A wide range of browsers supports using supports CSS at-rule to check whether a CSS property is usable in browsers. |
Related to #1845.
This PR updates the Vanilla and Snow White theme to make gaps, chooser, tabs and buttons display properly in bidirectional languages.