-
Notifications
You must be signed in to change notification settings - Fork 896
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
Tailwind & UI library: Improve RTL support #21909
Open
igorschoester
wants to merge
35
commits into
trunk
Choose a base branch
from
21887-rtl-support-for-ui-library
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
igorschoester
added
the
changelog: non-user-facing
Needs to be included in the 'Non-userfacing' category in the changelog
label
Dec 10, 2024
igorschoester
force-pushed
the
21887-rtl-support-for-ui-library
branch
from
December 10, 2024 12:24
40de914
to
ec130f6
Compare
Pull Request Test Coverage Report for Build 5752a29193345ecc8601ed81bb6c582a081ace8cDetails
💛 - Coveralls |
igorschoester
force-pushed
the
21887-rtl-support-for-ui-library
branch
6 times, most recently
from
December 17, 2024 11:04
35b98d4
to
8eae9d9
Compare
igorschoester
force-pushed
the
21887-rtl-support-for-ui-library
branch
2 times, most recently
from
December 18, 2024 10:03
2386521
to
7027483
Compare
We need tailwindcss >= 3.3.0 for the RTL support through logical properties (start/end)
* introduce `onClear` function to have a distinction between `null` value or clear intent from the user * move to classes for easier overrides * fix the big empty space after the input * using start/end for better RTL support * add some boolean variables to try and make it a bit clearer to what is going on
* top-2.5 is 0.625rem, meaning it is positioned slightly more to the top now * this makes it centered with the default input height
* use rem instead of px * use Tailwind instead of CSS
* fix stylesheet whitespace
* using modifier so it works too if the RTL is put ON the yst-root class
The portals rely on the HTML dir being accurate. This useEffect updates it. Removing the need for the dir on the Root
* plus no more negative margin without card text
E.g. more like bottom-start in the newer language
* border radius * spacing (border overlap)
* collapsible chevron * mobile menu close * list indentation
Overriding the `src` from the grunt-plugin-tasks config
* collapsible (alerts) * notices close * notices content padding/indent
Not picked up by the Grunt RTL task Already an existing problem outside of this PR
igorschoester
force-pushed
the
21887-rtl-support-for-ui-library
branch
from
December 19, 2024 10:20
7027483
to
c8d2630
Compare
igorschoester
changed the title
UI library: Improve RTL support
Tailwind & UI library: Improve RTL support
Dec 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
changelog: non-user-facing
Needs to be included in the 'Non-userfacing' category in the changelog
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Warning
Please don't merge yet. We need a matching Premium PR to prevent some clashing directions.
Context
Some sources:
-rtl
stylesheet: https://github.com/Yoast/wordpress-seo/blob/trunk/admin/class-admin-asset-seo-location.php#L77Note
Disclaimer: I'm not that familiar with RTL, trying to just fix the obvious.
There are nuances like a HTML
code
block that might needdirection: ltr
enforcing. However, due to me not knowing if that is always the case I left things like that out. Maybe @vraja-pro has a better view on those for a follow-up 😉Summary
This PR can be summarized in the following changelog entry:
onClear
to the Autocomplete. If needing to know the difference betweennull
value and the user clearing the value.nullable
.tailwindcss
from^3.2.4
to^3.3.7
. 3.3.0 is the version that is improving the RTL support through logical properties (start/end).Relevant technical choices:
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
The headings below are structured per commit, which is alphabetical per elements and then components.
This was just the easiest for me to go about the code changes. However, in the plugin, there is more to it than the code changes. Therefor, the latter ones are more per plugin page/section.
Important
It makes sense to activate our other add-ons too.
The instructions are mostly from what is changes.
However, please check for things that I might've missed too!!
Note
Please ignore LTR on
code
HTMLThey are added through WP style. I'm told this makes sense for code blocks.
Tip
Switching between RTL
RTL tester
plugin to get a button in the admin bar/wp-admin/options-general.php
in a new tab. Besides manually changing the language, you can also do the same by running some JS in the console (in case you can't read the language 😁 ):document.getElementById("WPLANG").value = "he_IL"; document.getElementById("submit").click()
document.getElementById("WPLANG").value = ""; document.getElementById("submit").click()
Tip
For devs:
vendor/yoast
folder and build (after building Free first).yarn workspace @yoast/ui-library storybook
Update tailwindcss and @tailwindcss/forms
Upgrading the packages in the plugin and all the tooling. And updating the style reset in the Root of the UI library. In theory this can impact everything inside the
yst-root
.Technically anything can happen here. But in reality I would not spend too much time here. Some quick regression tests should suffice.
Where is the
Root
from the UI library used?Yoast admin (all our JS/React pages)
Editors
Other
Refactor Autocomplete element
Specifically the icon at the "end" and when it can be cleared (nullable).
/wp-admin/admin.php?page=wpseo_dashboard
Fix loading button RTL
Specifically the distance from the spinner to the text of the button.
/wp-admin/admin.php?page=wpseo_page_settings
Fix checkbox RTL
Not actually used in the plugin. But specifically the distance between the label and the checkbox.
Fix validation input icon placement
Fix validation RTL
Good to test these together probably.
For the icon placement look out for the distance from the top of the validation icon to the top of the input.
For the other it is the distance that is created for the icon at the end of the input and the position of the icon itself.
/wp-admin/admin.php?page=wpseo_page_settings#/site-representation
$#%$#%
Fix file input RTL
Fix file import RTL
Combining these here. Only in Shopify and UI library storybook.
Look out for the labels text being in the expected direction.
Look out for the feedback header spacing (distance between the icons and progressbar):
Fix radio RTL
Specifically the distances between the radios and the labels. And the check in the inline block variant.
/wp-admin/admin.php?page=wpseo_page_settings#/site-representation
/wp-admin/admin.php?page=wpseo_page_settings#/site-basics
Fix select RTL
Fix select field story RTL
Combined checks. Not actually be used in the plugin yet.
The text is start aligned and the icon is end aligned.
For the story: the badge should be spaced away from the label
Fix skeleton loader story RTL
Only touched the UI library storybook part. No need to check the plugin.
Specifically, the spacing between the round profile and the rest.
Fix table RTL
The rounding of the header/row corners and the alignment of the header text.
/wp-admin/admin.php?page=wpseo_page_settings#/rss
Refactor tag input style
The height of the tags themselves and the padding at the end / "after" the remove (x) button.
/wp-admin/admin.php?page=wpseo_page_settings#/post-type/posts#input-wpseo_titles-page-analyse-extra-post
Fix toast description RTL
A toast description (list) should now have the distance between the discs and the edge correct in RTL.
Used in the Notification of the UI library, when passing a description. Which I don't think we use in a plugin.
Refactor toggle RTL
The handle when checked should move the correct way.
/wp-admin/admin.php?page=wpseo_page_settings#/site-features
(also found on the integrations page)Fix tooltip RTL
Fix toggle field description RTL
Combining these two changes.
The position of the tooltip compared to the parent, the arrow of the tooltip. Basically all the positioning.
For the toggle field: the distance from the description and the toggle itself should be correct.
/wp-admin/admin.php?page=wpseo_dashboard
Fix storybook preview HTML dir
Devs: for example in the modal story you would now get the modal itself in RTL too. You can check that the
html
in the iframe receives thedir
attribute as per the direction toggle.Fix feature upsell RTL
The lock icon moving slightly more to the border of the button (away from the text). And only for the storybook, without any text it does not move away anymore, so centered now.
/wp-admin/admin.php?page=wpseo_page_settings#/crawl-optimization#input-wpseo-deny_google_extended_crawling
Fix modal close RTL
The close button alignment should be towards the end.
/wp-admin/admin.php?page=wpseo_page_settings#/site-features
Fix notifications bottom-left in RTL
Should be already checked due to the previous steps. But you can repeat the X/Twitter field error to get a notification pop up again.
Fix pagination RTL
Not used in the plugin.
Look for the border radius/rounding and there should be no "double" borders.
Fix sidebar navigation RTL
Collapsible chevron icon spacing should be to the end.
The mobile menu close should be next to the menu (not under it?).
The indentation of the menu items should be correct.
/wp-admin/admin.php?page=wpseo_page_settings#/site-features
Exclude Tailwind from Grunt RTL build
This is potentially more invasive. As it affect ALL the previously thought fixed RTL things.
Some context. The Grunt RTL task basically reversed all the known LTR styles. This interferes with the standalone RTL of the UI library (as a package). So the goal is that this PR makes that RTL pass irrelevant. So please verify if that is true.
We need to go over basically everywhere we use the UI library/Tailwind style. I tried to do this and fix all the things I ran into, related to this change or not 😅
So please pay some extra attention to spacing, icons appearing in expected order, borders.
css/dist
folder does not include atailwind-xxx-RCx-rtl.css
file (note the-rtl
at the end).General page
/wp-admin/admin.php?page=wpseo_dashboard
Settings page
/wp-admin/admin.php?page=wpseo_page_settings
/wp-admin/admin.php?page=wpseo_page_settings#/site-features
learn more
arrows are pointing towards the "end", away from the copy/wp-admin/admin.php?page=wpseo_page_settings#/site-representation
Add another profile
is slightly towards the "start" of the buttonbreadcrumbs
in the language you are testing with as the search is translated (פירורי לחם
in Hebrew )ms
class here as that failed 🤯 )/wp-admin/admin.php?page=wpseo_page_settings#/crawl-optimization#input-wpseo-deny_google_extended_crawling
/wp-admin/admin.php?page=wpseo_page_settings#/site-basics#input-wpseo_titles-publishing_principles_id
/wp-admin/admin.php?page=wpseo_page_settings&d=rtl#/post-type/posts#input-wpseo_titles-page-analyse-extra-post
Installation page
/wp-admin/admin.php?page=wpseo_installation_successful_free
Academy page
/wp-admin/admin.php?page=wpseo_page_academy
Editor
Use AI
buttontext-right
on the semrush table tooltip. Will be fixed later by removing fixed widths so it relies on the justify-end.Integration page
/wp-admin/admin.php?page=wpseo_integrations
Network disabled
,New
Support page
/wp-admin/admin.php?page=wpseo_page_support
Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo]
, added test instructions for Shopify and attached theShopify
label to this PR.Documentation
Quality assurance
Innovation
innovation
label.Fixes #21887