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

fix: Add more RTL improvements #526

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

fix: Add more RTL improvements #526

wants to merge 10 commits into from

Conversation

fsbraun
Copy link
Member

@fsbraun fsbraun commented Mar 30, 2024

Description

This PR should improve admin stying further for RTL languages

  • Login panel
  • Tables
  • Changelist
  • Forms
  • Filer widgets

@sakhawy Can you give it a manual test run?

Related resources

Checklist

Copy link

codecov bot commented Mar 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (8edbd66) to head (4bede97).

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #526   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           33        33           
  Branches         3         3           
=========================================
  Hits            33        33           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@sakhawy sakhawy left a comment

Choose a reason for hiding this comment

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

Hi @fsbraun! I haven't looked into everything but I tried to point out what I think should be corrected!

Copy link
Contributor

Choose a reason for hiding this comment

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

For submit-row I think it should float to the left on rtl
Screenshot from 2024-03-30 15-48-42

@@ -80,16 +83,22 @@ body.login.djangocms-admin-style {
}
.submit-row {
float: right;
[dir="rtl"] & {
float: right;
Copy link
Contributor

Choose a reason for hiding this comment

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

float: left;

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, obviously ... 🤦‍♂️

djangocms_admin_style/sass/components/_login.scss Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this file be included?

djangocms_admin_style/sass/components/_icons.scss Outdated Show resolved Hide resolved
}
}
}

[dir="rtl"] .submit-row .deletelink-box, .deletelink {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think .deletelink should be excluded from floating to the right:

Before:
image

After:
image

djangocms_admin_style/sass/components/_base.scss Outdated Show resolved Hide resolved
djangocms_admin_style/sass/components/_base.scss Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the text is not floating right:
image

I think it should start from the right direction here:
image

I'm not sure if this is the right file for this issue, but the navigator isn't quite right:
image
image

For reference here's how it looks in LTR:
image

image

@fsbraun
Copy link
Member Author

fsbraun commented Mar 30, 2024

@sakhawy Thanks for the excellent feedback. I fixed most issues

  • Icons is reverted
  • floats now make sense (I hope)

The filer issues probably need to be fixed in the filer repo itself - probably both css and js.

@fsbraun fsbraun requested review from sakhawy and a team and removed request for sakhawy April 2, 2024 19:10
Copy link
Contributor

@sakhawy sakhawy Apr 5, 2024

Choose a reason for hiding this comment

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

Screenshot from 2024-04-05 16-43-58
Screenshot from 2024-04-05 16-43-53

Line 49 in _icons.sass should be changed to margin-inline-end I think? You changed it back to margin-right in another PR I don't know why.

EDIT: I remember that I requested this because other icons had padding in them(?) but it looks like it backfired in the dashboard.

Also, sorry I didn't mean to notify you right away.

Copy link
Contributor

@sakhawy sakhawy left a comment

Choose a reason for hiding this comment

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

Thanks @fsbraun! I did some manual testing on most of the components and it looks like djangocms-admin-style will be good to go after this PR and some changes to _tables.scss and _sideframe.scss!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think something went wrong with select2-drop
Screenshot from 2024-04-05 17-04-14
image
image

For reference:
image

Notice the search icon, the remove icon, the border before the arrow, the arrow, and the width of the container.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

There should be an arrow on the left

For reference:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

The padding on lines: 134, 156, and 255.
The margin on line: 155.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should changes to _changelist.scss be accompanied by changes to _tables.scss?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the icons for choosing permissions should be mirrored:
Screenshot from 2024-04-05 17-53-46

And the links (for choosing all and removing all) should be moved to the right. (_content.scss)

Copy link
Contributor

Choose a reason for hiding this comment

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

calendarnav-previous and calendarnav-next should be swapped (?) -- they're working fine, the direction of the icons is correct, but the place is not.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why padding in line 18 and margin-left in line 24 were left out?

Copy link
Contributor

Choose a reason for hiding this comment

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

The padding on lines: 46 and 90

Copy link
Contributor

Choose a reason for hiding this comment

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

The margin on lines 337 and 684.
The padding on lines 450 and 788.

Copy link
Contributor

Choose a reason for hiding this comment

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

border-radius on lines 131 and 136
padding-left on line 201.
padding on line 329.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants