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(theme): add rtlcss directive #1512

Merged
merged 8 commits into from
Nov 7, 2022

Conversation

sadeghbarati
Copy link
Contributor

@sadeghbarati sadeghbarati commented Oct 19, 2022

https://vitepress-rtlcss.vercel.app/

This is just simple rtlcss directive

Still requires to User to add rtlcss as their devDependencies and use it in vitepress config,
Because it has not been implemented in node folder

// .vitepress/config

import { defineConfig } from 'vitepress'
import rtlcss from 'rtlcss'

export default defineConfig({
  lang: 'en-US',
  title: 'VitePress',
  description: 'Vite & Vue powered static site generator.',
  
  vite: {
    postcss: {
      plugins: [
        rtlcss()
      ]
    }
  }
})

I saw i18n and i18n-next branch for handling dir and lang options and other feats,

If you add dir frontmatter would be so good, documentation sites support both RTL and LTR at the same time

page-rtl.md

---
dir: 'rtl'
---

RTL Content

page-ltr.md

---
dir: 'ltr'
---

LTR Content

@brc-dd
Copy link
Member

brc-dd commented Oct 24, 2022

If you add dir frontmatter would be so good, documentation sites support both RTL and LTR at the same time.

I can add that, but I don't think it will be necessary as the current implementation (i18n-next) already allows overriding dir based on locale. So, if you have some structure like this:

- foo.md
- fa/
--- foo.md

in your config you can override dir and lang for whole fa/ directory. This way /foo can serve pages with ltr direction and fa/foo can serve pages in rtl direction at the same time.


Regarding your PR, I don't have much experience, so if someone can confirm this works fine (or if you can provide a demo/screen rec. with an rtl page) I'll merge it. (sorry, I didn't saw the link at the very top, I'll review it now).

@brc-dd
Copy link
Member

brc-dd commented Oct 24, 2022

x-ref: #631, #902


@xfq, @fvsch Hi guys! Can you take a look if this sorts out issues with the RTL layout, and point out if something seems missing?

@brc-dd brc-dd added enhancement New feature or request theme Related to the theme labels Oct 24, 2022
@xfq
Copy link
Contributor

xfq commented Nov 5, 2022

"K⌘" in the search bar should be "⌘K" (on macOS). The same for other operating systems.

I didn't see any other obvious problems, but I think it would be useful to have an Arabic/Hebrew/etc.-speaking expert double-check the page and/or the code.

@brc-dd brc-dd changed the base branch from main to feat/i18n-next November 7, 2022 15:15
@brc-dd brc-dd merged commit b4e1483 into vuejs:feat/i18n-next Nov 7, 2022
@brc-dd
Copy link
Member

brc-dd commented Nov 7, 2022

I'm merging this with i18n(-next) branch for now. We can tackle other issues directly on #1339.

Regarding documenting both ltr and rtl in the same app, there will be some issues as the styles will be shared, so if you add the rtlcss postcss plugin, the styles for ltr pages will also change.

@brc-dd brc-dd mentioned this pull request Nov 7, 2022
12 tasks
@sadeghbarati sadeghbarati deleted the feat/rtlcss branch November 13, 2022 09:13
@brc-dd
Copy link
Member

brc-dd commented Nov 18, 2022

there will be some issues as the styles will be shared

Regarding this, I wanted to ask are you using override mode in postcss-rtlcss (at https://vitepress-rtlcss.vercel.app/)? Because according to its docs, the default is the combined mode, where ltr and rtl styles are separate (prefixed). I'm guessing that it somehow messes up with the specificity of selectors?

@sadeghbarati
Copy link
Contributor Author

Hi,
as I mentioned before I'm using rtlcss not postcss-rtl, rtlcss package will transforms all styles from RTL to LTR or vice versa

rtlcss:

/* before */
body {
  direction: ltr;
}
/* after */
body {
  direction: rtl;
}

but postcss-rtl will generate another style besides of ltr or rtl style, It can increase the size of CSS and also change selectors specificity

postcss-rtl:

/* before */
body {
  direction: ltr;
}
/* after */
body {
  direction: ltr;
}
[dir="rtl"] body {
   direction: rtl;
}

@sadeghbarati
Copy link
Contributor Author

sadeghbarati commented Nov 18, 2022

I didn't check the vitepress node folder completely

const stylesheetLink = cssChunk
? `<link rel="stylesheet" href="${siteData.base}${cssChunk.fileName}">`
: ''

When you are using rtlcss you have to generate CSS two times (.vitepress/dist/assets), one for ltr without any rtlcss proccess (style.********.css) and another with rtlcss (style-rtl.********.css)

since vitepress is SSR I thought it can handle two stylesheets based on locale directories

- foo.md => load ltr stylesheet
- fa/
--- foo.md => load rtl stylesheet

@brc-dd
Copy link
Member

brc-dd commented Nov 18, 2022

as I mentioned before I'm using rtlcss not postcss-rtl

Ah, right. I was actually looking at the docs of https://www.npmjs.com/package/postcss-rtlcss

change selectors specificity

Yeah, that's what I was thinking.

since vitepress is SSR I thought it can handle two stylesheets based on locale directories

I can make it work, but it will need a full page reload for styles to apply.

@sadeghbarati
Copy link
Contributor Author

I can make it work, but it will need a full page reload for styles to apply

Ow you're right I thought it would work

I think CSS Logical Properties is the propper way to achieve i18n goal then, however support table is colorful 😵

@brc-dd
Copy link
Member

brc-dd commented Nov 19, 2022

On second thoughts, we can use :where([dir="rtl"]) as the prefix, it will keep the specificity same, and apply appropriate styles when the page is rtl.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request theme Related to the theme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants