Skip to content

[4.0] Align sidebar icon#27156

Merged
infograf768 merged 8 commits intojoomla:4.0-devfrom
ciar4n:side-icon-align
Nov 27, 2019
Merged

[4.0] Align sidebar icon#27156
infograf768 merged 8 commits intojoomla:4.0-devfrom
ciar4n:side-icon-align

Conversation

@ciar4n
Copy link
Contributor

@ciar4n ciar4n commented Nov 26, 2019

Pull Request for Issue #27135 .

Summary of Changes

Moves sidebar icons to using flex. Fixes alignment issue.

Testing Instructions

Apply this patch and run node build.js --compile-css for updating the changed SCSS.

Check sidebar icons are aligned.

Before PR

image

With PR

image

Documentation Changes Required

@infograf768
Copy link
Member

This totally breaks RTL...
Screen Shot 2019-11-26 at 11 02 05

@infograf768
Copy link
Member

FYI, code for rtl is in template-rtl.scss

transition: all .3s ease-out;
.has-arrow {
.sidebar-item-title {
margin-right: auto;
Copy link
Member

@C-Lodder C-Lodder Nov 26, 2019

Choose a reason for hiding this comment

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

& [dir=rtl] {
  margin-left: auto;
}

this might fix the RTL issue @infograf768 mentioned

@infograf768
Copy link
Member

Looks like the issue is partly
.main-nav .has-arrow .sidebar-item-title {
margin-right: auto;
}

which should be
margin-left: auto;
in RTL
but this not sufficient to solve the position of the arrows

@ciar4n
Copy link
Contributor Author

ciar4n commented Nov 26, 2019

Should be ok now...

image

@infograf768
Copy link
Member

RTL solved.

Another issue with the home multilingual menutypes

before:
Screen Shot 2019-11-26 at 11 54 53

after
Screen Shot 2019-11-26 at 11 56 50

the badges are added in
mod_menu/tmp/default_submenu.php line 113
and
mod_submenu/Menu/Menu.php line 176

@ciar4n
Copy link
Contributor Author

ciar4n commented Nov 26, 2019

Thank you. Badges fixed.

@Quy
Copy link
Contributor

Quy commented Nov 26, 2019

I have tested this item ✅ successfully on 697f52f


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

@infograf768
Copy link
Member

Will test later. Maybe worth adding all rtl specific overrides in the sidebar.scss as I did already for other rtl stuff in various PRs in the recent weeks.

@infograf768
Copy link
Member

infograf768 commented Nov 27, 2019

@ciar4n

I propose this patch which includes your changes but moves to _sidebar.scss all main-nav rtl stuff (improved) and deletes it from template-rtl.scss.
If you agree, could you update your PR?

navbar_rtl.diff.zip

@ciar4n ciar4n closed this Nov 27, 2019
@ciar4n ciar4n reopened this Nov 27, 2019
@infograf768
Copy link
Member

@ciar4n
I modified the .zip above to indent correctly the arrow in submenu

+[dir=rtl] .main-nav ul .dropdown-submenu a {
+  padding: 0 2.0rem 0 1rem;

Test can be done using the alternate preset.

@ciar4n
Copy link
Contributor Author

ciar4n commented Nov 27, 2019

@infograf768

I have moved the rtl to the sidebar.scss as suggested. Also removed some redundant css.

@infograf768
Copy link
Member

@ciar4n
Your patch is working but I wonder about

      a {
        [dir=ltr] & {
          padding-left: 3rem;
        }

        [dir=rtl] & {
          padding-right: 3rem;
        }

and similar further

      a::before {
        [dir=ltr] & {
          left: 1.25rem;
        }

        [dir=rtl] & {
          right: 1.25rem;
        }
      }

Do we need the [dir=ltr] or is it there to just prevent adding a padding-right: 0 in the rtl part?

@ciar4n
Copy link
Contributor Author

ciar4n commented Nov 27, 2019

Do we need the [dir=ltr] or is it there to just prevent adding a padding-right: 0 in the rtl part?

Yes exactly. It avoids having to correct the ltr css in the rtl.

Personally I find defining both ltr and rtl a little cleaner and easier to read. It also makes me more rtl aware (which I tend to forget about).

Alas I am happy to change it to whichever you prefer?

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 9166828


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

@infograf768
Copy link
Member

I like the idea. Cleaner indeed. Rtc.


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 27, 2019
@infograf768 infograf768 merged commit 4f5916c into joomla:4.0-dev Nov 27, 2019
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 27, 2019
@infograf768
Copy link
Member

Tks!

@infograf768 infograf768 added this to the Joomla 4.0 milestone Nov 27, 2019
@ciar4n ciar4n deleted the side-icon-align branch November 27, 2019 16:27
@ciar4n
Copy link
Contributor Author

ciar4n commented Nov 27, 2019

Thank you for the tests

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.

5 participants