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

[4.0] Improve Menu/Toolbar RTL in atum #21136

Merged
merged 21 commits into from
Jul 16, 2018
Merged

Conversation

wilsonge
Copy link
Contributor

@wilsonge wilsonge commented Jul 15, 2018

Improves the menu (text is now on the right not left of the menu), and the toolbar (things now render semi-correctly). The table options drop down also now opens correctly below the dropdown

@ciar4n please review me trying to implement CSS - I'm sure its dreadful in more than one way :)

Test by installing the Persian Language Pack

.ml-auto, .mx-auto {
margin-left: 0 !important;
margin-right: auto !important;
}

Choose a reason for hiding this comment

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

Files should end with a trailing newline

Copy link
Contributor

Choose a reason for hiding this comment

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

Try an avoid changing Bootstrap utility classes in the template-rtl.css. Flex is RTL by default so should be no need for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

}

.ml-auto, .mx-auto {
margin-left: 0 !important;

Choose a reason for hiding this comment

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

Properties should be ordered margin-right, margin-left

}

.js-stools .js-stools-container-filters {
left: 0;

Choose a reason for hiding this comment

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

Properties should be ordered right, left

@infograf768
Copy link
Member

MUCH better!

We also need in template-rtl
.main-nav .fa{margin:0 18px} // same as LTR

to get
screen shot 2018-07-16 at 08 41 18

instead of
.main-nav .fa{margin:0 12px 0 8px}

screen shot 2018-07-16 at 08 40 54

}

.js-stools .js-stools-container-filters {
left: 0;

Choose a reason for hiding this comment

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

Properties should be ordered right, left


.main-nav {
.fa{
margin:0 18px

Choose a reason for hiding this comment

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

Declaration should be terminated by a semicolon
Colon after property should be followed by at least one space

@@ -226,3 +227,23 @@ th {
}
}
}

.main-nav {
.fa{

Choose a reason for hiding this comment

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

Opening curly brace { should be preceded by one space

@ciar4n
Copy link
Contributor

ciar4n commented Jul 16, 2018

Delete.. https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/templates/atum/scss/template-rtl.scss#L45

Not needed and pushing menu out form edge of viewport

@@ -1714,6 +1709,12 @@ th {
left: 15px;
content: "\f0d9"; }

.main-nav li a span {
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.

Once you change https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/templates/atum/scss/blocks/_sidebar.scss#L84 to flex, remove this. Floats are ignored in flex objects.

@ciar4n
Copy link
Contributor

ciar4n commented Jul 16, 2018

https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/templates/atum/scss/template-rtl.scss#L118

Change to right: 60px for correct position of sub menu

@ciar4n
Copy link
Contributor

ciar4n commented Jul 16, 2018

As mentioned by @infograf768, remove the following... https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/templates/atum/scss/template-rtl.scss#L111-L113

@wilsonge
Copy link
Contributor Author

Change to right: 60px for correct position of sub menu

Ahh that's the one I was looking for but couldn't find :) Thanks!

I've also simplified down the toolbar a bit more. So with these corrections. Any objections to merging this? It's far from perfect but much better than before :P

.btn-toolbar > * {
margin-left: 0;

Choose a reason for hiding this comment

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

Properties should be ordered margin-right, margin-left

@ghost
Copy link

ghost commented Jul 16, 2018

I have tested this item ✅ successfully on 2f186ae


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21136.

.mr-auto {
margin-right: 0 !important;
margin-left: auto !important;
}

Choose a reason for hiding this comment

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

Files should end with a trailing newline

@wilsonge
Copy link
Contributor Author

One last bug fix for the top menu 😇

@infograf768
Copy link
Member

If you can, let's also correct this

screen shot 2018-07-16 at 13 24 48

.mr-2, .mx-2 {
    margin-right: .5rem !important;
}

Normally if I understand well scss
it should be in LTR

.mr-2,
.mx-2 {
  margin-right: 0.5rem !important; }
 margin-left: 0;  
}

and in RTL

.mr-2,
.mx-2 {
    margin-right: 0;
    margin-left: .5rem !important;
}

@wilsonge
Copy link
Contributor Author

@infograf768 Which view is this in?

@wilsonge
Copy link
Contributor Author

Don't worry - found it and done

.mr-2 {
margin-left: 0.5rem;
margin-right: 0 !important;
}

Choose a reason for hiding this comment

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

Files should end with a trailing newline

}

.mr-2 {
margin-left: 0.5rem;

Choose a reason for hiding this comment

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

Properties should be ordered margin-right, margin-left
0.5 should be written without a leading zero as .5

@wilsonge wilsonge changed the title Improve Menu/Toolbar RTL in atum [4.0] Improve Menu/Toolbar RTL in atum Jul 16, 2018
@infograf768
Copy link
Member

Articles manager

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 04d3851

Good to go for now. Solves a lot of stuff. If we find something missing, we can add issue later.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21136.

@laoneo
Copy link
Member

laoneo commented Jul 16, 2018

@franz-wohlkoenig can you also retest please.

@ghost
Copy link

ghost commented Jul 16, 2018

@laoneo will do in next Minutes.

@@ -160,38 +160,29 @@ th {
// Toolbar
.subhead {

.btn-group-sm > .btn {
.btn, .btn-sm {
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the .btn-sm from here as it's an addition to .btn which is required anyway

@ghost
Copy link

ghost commented Jul 16, 2018

@infograf768 this for another Issue?

screen shot 2018-07-16 at 17 26 27
screen shot 2018-07-16 at 17 26 09

@wilsonge wilsonge merged commit 68448c3 into joomla:4.0-dev Jul 16, 2018
@wilsonge wilsonge deleted the feature/rtl-atum branch July 16, 2018 15:54
@wilsonge
Copy link
Contributor Author

Yah there's a huge amount more to do here. But let's take it one step at a time :)

@wilsonge wilsonge added this to the Joomla 4.0 milestone Jul 16, 2018
@infograf768
Copy link
Member

infograf768 commented Jul 17, 2018

Note: for the cpanel
.flex-grow-1 is missing a text-align: right;
.list-group is also missing text-align: right;

@C-Lodder
Copy link
Member

Don't add it to flex-grow-1. This class instructs the element to fill available space and will do so regardless of RTL

@infograf768
Copy link
Member

Indeed
list-group is enough

@wilsonge
Copy link
Contributor Author

With the merge of #20962 that's not required for now as they've been moved to tables

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.

None yet

7 participants