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: Add RTL support to admin modal #525

Merged

Conversation

sakhawy
Copy link
Contributor

@sakhawy sakhawy commented Mar 18, 2024

Description

Added RTL support to cms-admin-modal

Related resources

Screenshots

Screenshot from 2024-03-18 11-10-57
Screenshot from 2024-03-18 11-11-03
Screenshot from 2024-03-21 13-33-02

Checklist

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7b7e5e5) to head (87c43e9).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #525   +/-   ##
=========================================
  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.

@sakhawy sakhawy force-pushed the feat/add-RTL-support-to-admin-modal branch from 9563c42 to 39cc245 Compare March 21, 2024 11:42
@sakhawy sakhawy marked this pull request as ready for review March 21, 2024 11:43
@sakhawy sakhawy force-pushed the feat/add-RTL-support-to-admin-modal branch from 39cc245 to 87c43e9 Compare March 21, 2024 11:55
@fsbraun
Copy link
Member

fsbraun commented Mar 21, 2024

@sakhawy Are the buttons and header positioned correctly?

@sakhawy
Copy link
Contributor Author

sakhawy commented Mar 21, 2024

Oh, I missed that! I'll adjust it, thanks!

@sakhawy
Copy link
Contributor Author

sakhawy commented Mar 21, 2024

@fsbraun Should I make this adjustment from djangocms-admin-style? Or would it better be a django-cms PR since the styling is in django-cms/cms/static/cms/sass/components/_modal.scss?

https://github.com/django-cms/django-cms/blob/21d6a6defc5a6321bd691e43eb57c112508b952f/cms/static/cms/sass/components/_modal.scss#L150-L170

.cms-modal-minimize,
.cms-modal-close,
.cms-modal-maximize {
    display: block;
    position: absolute;
    top: 50%;
    margin-top: math.div(-$modal-header-button-area-size, 2);
    right: $padding-normal;
    color: $gray-light;
    text-align: center;
    width: $modal-header-button-area-size;
    height: $modal-header-button-area-size;
    cursor: pointer;
    &:before {
        position: relative;
        top: math.div($modal-header-button-area-size - $icon-size, 2);
    }
    &:hover {
        color: $color-primary;
    }
}

@fsbraun
Copy link
Member

fsbraun commented Mar 21, 2024

You're right, of course.

@@ -305,7 +308,12 @@ form {
float: left !important;
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't that be float: start?

@@ -66,12 +66,17 @@
float: left;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
float: left;
float: start;

@@ -143,15 +150,21 @@
float: left;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
float: left;
float: start;

@@ -161,13 +174,23 @@
display: inline-block;
float: left;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
float: left;
float: start;

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.

I think you can use float: start and float: end...

@sakhawy
Copy link
Contributor Author

sakhawy commented Mar 22, 2024

@fsbraun inline-start and inline-end are not supported on relatively older browsers (as recent as 2023): https://caniuse.com/mdn-css_properties_float_inline-start so, I wanted to be extra careful. Should I make the change?

@fsbraun fsbraun merged commit f1bbc9d into django-cms:master Mar 26, 2024
21 checks passed
@fsbraun fsbraun mentioned this pull request Mar 30, 2024
3 tasks
@joshyu
Copy link

joshyu commented Apr 2, 2024

@fsbraun , I think it's not a best practice to use too many !important in the CSS codes, which prevent us from customize those styes easily.

@fsbraun
Copy link
Member

fsbraun commented Apr 2, 2024

@joshyu I fully agree. djangocms-admin-style is not a good example for this, but has gone down that road may years ago. Alas, I believe with a bit of work this is fixable in most cases since all admin pages have the class djangocms-admin-style added to them allowing to override other styles by using higher specificity. It would be great if someone addressed this.

As a simpler work-around, I have created my own djangocms-simple-admin-style project (available on pypi): It tries to keep as many original django styles as possible and to void !important... Downside: It only works on browsers supporting CSS nesting (https://caniuse.com/css-nesting)

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.

3 participants