8000 feat: Add RTL support to modal header and related components by sakhawy · Pull Request #7863 · django-cms/django-cms · GitHub
[go: up one dir, main page]
Skip to content

feat: Add RTL support to modal header and related components #7863

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 23, 2024
Merged

feat: Add RTL support to modal header and related components #7863

merged 3 commits into from
Apr 23, 2024

Conversation

sakhawy
Copy link
Contributor
@sakhawy sakhawy commented Apr 1, 2024

Description

Followup to #525.

I touched related files for style consistency.

Related resources

Screenshots

Screenshot from 2024-04-01 10-45-16
Screenshot from 2024-04-01 10-45-28
Screenshot from 2024-04-01 10-45-33

Checklist

  • I have opened this pull request against develop-4
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on Slack to find a “pr review buddy” who is going to review my pull request.

@fsbraun fsbraun self-assigned this Apr 1, 2024
@joshyu
Copy link
joshyu commented Apr 2, 2024 •
edited
Loading

@fsbraun / @sakhawy ,
I found a post which is related to how to style RTL mode https://rtlstyling.com/posts/rtl-styling

fsbraun
fsbraun requested changes Apr 2, 2024
View reviewed changes
Copy link
Member
@fsbraun fsbraun left a comment

Choose a reason for hiding this comment

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

@sakhawy This is cool stuff! Looks great.

I have two comments for the structure board.

Also, I can imagine going one last step further: Also make the toolbar RTL, i.e. the structure board on the left, the admin sidebar on the right and the menu from rtl. What do you think?

@sakhawy
Copy link
Contributor Author
sakhawy commented Apr 2, 2024

Thanks for the review @fsbraun! The toolbar is the very next thing on my list but is it good for the scope of this PR? Should I open up another one or just make the changes here?

@fsbraun
Copy link
Member
fsbraun commented Apr 2, 2024

@sakhawy You can surely do this in a separate PR. Then I'd suggest a different approach for putting the dir attribute into toolbar.html:

  1. Remove both lang and dir attributes from the outmost <div id="cms-top">.
  2. Add dir="ltr" to <div class="cms-toolbar">, forcing the acutal toolbar to be ltr (as opposed to the whole frontend editing interface)

This way all the other components (modal, structure board, ...) will use the direction of the document. The toolbar itself will be forced to be ltr.

This dir="ltr" will need to be removed once, we add rtl support to the toolbar itself.

What do you think?

@sakhawy
Copy link
Contributor Author
sakhawy commented Apr 5, 2024

Following the plan, since the modal inherits dir from toolbar.html, I added

        lang="{{ LANGUAGE_CODE|default:"en-us" }}" 
        dir="{{ LANGUAGE_BIDI|yesno:'rtl,ltr,auto' }}"

to <div class="cms-modal" ...></div> in toolbar_with_structure.html and will remove it once RTL support is added to toolbar.

fsbraun
fsbraun approved these changes Apr 9, 2024
View reviewed changes
Copy link
Member
@fsbraun fsbraun left a comment

Choose a reason for hiding this comment

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

LGTM!

This was referenced Apr 13, 2024
Merged
Merged
@fsbraun
Copy link
Member
fsbraun commented Apr 23, 2024

@sakhawy Can you check if this change is still up-to-date?

django-cms/cms/templates/cms/toolbar/toolbar_with_structure.html

Lines 37 to 44 in 898a14a

<!-- TODO: Remove lang and dir once RTL support is added to toolbar -->
<div
class="cms-modal"
tabindex="-1"
data-touch-action="none"
lang="{{ LANGUAGE_CODE|default:"en-us" }}"
dir="{{ LANGUAGE_BIDI|yesno:'rtl,ltr,auto' }}"
>

If so, I will merge and include the PR in django CMS 4.1.1.

@sakhawy sakhawy force-pushed the feat/add-RTL-support-to-modal-and-related branch from 898a14a to 1bf9c6a Compare April 23, 2024 12:58
@sakhawy
Copy link
Contributor Author
sakhawy commented Apr 23, 2024

@fsbraun I removed the dir attribute from all the child divs. The elements should now inherit the base direction from the html tag in djangocms_frontend.html.

@fsbraun fsbraun merged commit bef0045 into django-cms:develop-4 Apr 23, 2024
@fsbraun fsbraun mentioned this pull request May 2, 2024
Closed
4 tasks
fsbraun pushed a commit to fsbraun/django-cms that referenced this pull request May 2, 2024
@sakhawy @fsbraun
feat: Add RTL support to modal header and related components (django-…
67b98d6 
…cms#7863)
@fsbraun fsbraun mentioned this pull request May 2, 2024
Closed
4 tasks
fsbraun added a commit that referenced this pull request May 12, 2024
@fsbraun @sakhawy @wfehr
feat: Toolbar direction set based on toolbar language (back port of #…
6c2a0be 
…7902) (#7914)

* feat: Add RTL support to toolbar (#7871)

* feat: Add RTL support to toolbar

* fix: Add logical text-align value for toolbar dropdown

* fix: Correctly calculate available width of toolbar

* fix: Fix weird floating bug with toolbar more buttons

* feat: Add RTL support to modal header and related components (#7863)

* fix: structure board on the right for ltr

* Fix: also use key-length of 200 for the actual cache-key of placeholders

- missing "functionality" for changes of #7595 and #7657

* fix: Set toolbar direction based on toolbar language (#7902)

* Fix: Merge remnant

* Update make-release script

---------

Co-authored-by: Moe <mohammadalsakhawy@gmail.com>
Co-authored-by: Wolfgang Feh
6AD1
r <24782511+wfehr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fsbraun fsbraun fsbraun approved these changes

Assignees

@fsbraun fsbraun

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sakhawy @joshyu @fsbraun
0