@dbhurley Is this B/C save? IIRC we can't remove/rename a lang strings in 3.x or i'm wrong here?

This was just committed a couple days ago, no release has gone out with the strings.

ok sorry yes you are right.

@test
I've just noticed this "closing cross" icon while testing another PR, and I was confused. Looking better now, Thanks!

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

@test: Failed. The original icons were chosen because they are agnostic. The problem with the current icons is that they are wrong in RTL languages.

I find the floating arrow kind of weird, having a background like in the original PR made more sense since it was linked to the sidebar items.

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

I don't know if you can say a test fails because there's a disagreement
over icon usage. ;)

Or because one finds the UI "weird"

However, the proper solution would be an implementation of the reverse
arrows in the rtl template files. This would be the best use case of proper
UI... Not the "hamburger" menu icon which is not representative of an
open/close for secondary navigation or widely recognized as such.

Just my two cents. These icons are worldwide recognized for open/close and
merely need to be updated for the rtl.
On Oct 4, 2014 6:45 PM, "RolandD" notifications@github.com wrote:

@test https://github.com/test: Failed. The original icons were chosen
because they are agnostic. The problem with the current icons is that they
are wrong in RTL languages.

I find the floating arrow kind of weird, having a background like in the
original PR made more sense since it was linked to the sidebar items.

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/4450
http://issues.joomla.org/tracker/joomla-cms/4450.

—
Reply to this email directly or view it on GitHub
#4450 (comment).

Oh the icon disagreement has been there since day 1, I had arrows in the very first design ;) Now I don't care anymore as long as they are correct. Since they are not correct in RTL, that is why I failed the test.

So the usage of the word "weird" is not correct in describing that I dislike the new UI. Then just I don't like the new design.

@test

Patch works fine.

When the sidebar is collapsed, the arrow is black / grey when hovered. However, when expanded, it is grey / black when hovered. I'd change it so that is is always black / grey when governed so that it always appears active and is consistent with the other elements.

I'd also suggest it have the same animation as Search tools show/hide.

Good catch on colors. And yes, I agree about the animation.

I'll update the PR.
On Oct 4, 2014 7:35 PM, "Matt Thomas" notifications@github.com wrote:

@test https://github.com/test

Patch works fine.

When the sidebar is collapsed, the arrow is black / grey when hovered.
However, when expanded, it is grey / black when hovered. I'd change it so
that is is always black / grey when governed so that it always appears
active and is consistent with the other elements.
[image: screenshot - 10042014 - 02 35 14 pm]
https://cloud.githubusercontent.com/assets/634119/4516413/3c1d2dfc-4bf5-11e4-9aba-fcfc0337f9ec.png

I'd also suggest it have the same animation as Search tools show/hide.

—
Reply to this email directly or view it on GitHub
#4450 (comment).

This does not make sense in RTL, as Roland says above.

Open

Close

We could use the correct arrows if we can check the language direction in the js

Updated for language direction in javascript. However I noticed this has bad styling as we are using bootstrap span classes directly within a bootstrap span. We need to have a boostrap row class prior to implementing spans otherwise the spacing is off. (You can see this by expanding/collapsing the sidebar and viewing the right side of the main column). This will need to be fixed before I would consider this functionality ready for production.

I agree that this functionality is not ready for production. (and that #4197 was not completely improved)
@dbhurley About the "hamburger" menu icon, i've proposed this one as generally used to open/hide a menu as a conventional used icon for this purpose. But, usage of arrows, with RTL/LTR switch as you have propose is a nice idea, if design gives a real slide sidebar effect. This is maybe what could be improved now IMHO.
In all cases, this feature is a good initiative from @roland-d

I am just surprised these comments were not made on the original PR. Why even merge it?

@roland-d If you are referring to my comment I have to say that I’ve always supported the idea of vanilla js in core.js: #4197 (comment)

@roland-d I merged it because it's something useful that (as I said in your PR) people will improve. I think it's globally a good code and a great feature.

I agree with @dbhurley in the span classes (as I mentioned in your PR). We should use a standard class that is mapped in the less files to the span.

Let's contribute all the fixes here

I'm the only one, using com_patchtester, who #4450 request me to have togglesidebar.php already in place, but if i apply #4197 with this file, after, i can't apply 4450 because of a language file already changed ?!?...
Just a question, if there's a solution to speed up my testing... ;-)

@JoomliC you can install the current 3.3.7 branche see: http://developer.joomla.org/cms-packages/ and after this apply this PR via com_patchtester.

index.php?option=com_cache&view=purge

needs to be fixed

@zero-24 thank you! Yes, this patch needs to be in 3.3.7-dev to apply with com_patchtester
@810 What did you mean ?

@dbhurley i've tested design about background, and yes, better when simpler! ;-)
I'm still convince that a slide effect would give improvement, but after a reflexion on the UX, i wonder now why header (for toolbar buttons) is in the admin template, and so loaded before content, and the sidebar in component view, and loaded after...
Isn't sidebar the place mainly used as sub-menus ?
My test is mainly done on mobile device, and i think it's a good way to see what is the most logical...

Couldn't it be simpler if the sidebar is moved to the admin template, and so not included in the content (more logical, no?)

@JoomliC Sorry I saw a conflict with collapsing the sidebar. But was a browser cache issue. Now it works perfectly

@test
+1

Like it! @test OK

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

2 successful tests.
ready to commit

Nope. Please read above: #4450 (comment)
there are still some issues

#4450 (comment)

Has to be also done in isis template_rtl.less

We also have an issue in RTL for this PR when hiding sidebar:

@dbhurley can you look at the RTL issues please. I am resetting the tests to zero again so that we only count new tests AFTER the RTL issues in this PR have been resolved.

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

@test success +1

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

You can't say it is a success, as stated above... We have at least 3 issues

it meaning i love this :D

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

Moving to RTC state as having more than 2 successful tests

As stated by @infograf768 it can't go to RTC, there are issues to be fixed.

In the mean while, I am now correcting to
#j-sidebar-container {
margin-right: 0;
}
as it is gets really bothersome to test stuff.
490c610

Just to give a possible little improvement, based on previous comments (#4197 (comment), #4197 (comment) ... )

This is a test (and would need improvements), with less code changes, using Bootstrap that does a much smoother expand/collapse.
(video with a "big" button, but could be achieved with the arrows as well)

https://www.youtube.com/watch?v=ErMILVuPneA&feature=youtu.be

@JoomliC Had a look at the video but still unsure about a few things:

• The video has no view that uses filters
• The video has no view that uses no search bar (thus the button uses extra screen space)

I do like the way the sidebar looks though, I don't think we need a textual button. There are enough images that can be used so people know what it does.

Hi @roland-d, sorry for slow response...
Sure you enjoy your last hours at Cancun ;-)

So, i've improved the design, with icons, and i have recorded a longer video, with different view (filters, no search bar...)
All feedback are welcome!

Video preview: https://www.youtube.com/watch?v=PF8zRRTSD_A

Screenshots open/close:

And if needed, should i open a new PR ?
Note that i've not yet worked on mobile devices and on RTL language, as it's just a test for the moment, but can do this next week! ;-)

@JoomliC Thanks, Cancun was a lot of fun. So I looked at the video and I do like what I see. We will require a new PR for that since you can't change this one. @dbhurley seems to have left it for what it is for now. This PR does no longer patch either. I would suggest a new PR is created and this one closed, so we can continue working on that and get it merged. OK for you David?

I wonder if issue joomla-projects/com_localise#233 is still a problem then.

@roland-d Roland congrats for your new role!

Thank you @dgt41

@roland-d @JoomliC
In com_localise I have overriden the sidebar/submenu layout to take in account this PR here.

<div class="toggle-sidebar">
    <?php if (JFile::exists(JPATH_ROOT . '/layouts/joomla/searchtools/default/togglesidebar.php')) : ?>
        <?php echo JLayoutHelper::render('joomla.searchtools.default.togglesidebar'); ?>
    <?php else : ?>
        <?php echo JLayoutHelper::render('joomla.sidebars.toggle'); ?>
    <?php endif; ?>
</div>

Therefore any PR that will be consistent with these layouts path and names should be OK.

The matter of the 2 closing divs I took off there is linked to the fact that our filters are not displayed through JHtmlSidebar::addFilter()

@roland-d and @dbhurley, yes i can open a new PR, maybe tomorrow.
I've improved the code this week-end, and i have added RTL behaviour.

@infograf768 I'm using the same layout as defined by @dbhurley : joomla.sidebars.toggle
So, it may not give issue with com_localise
And should work with joomla-projects/com_localise#233 (to be tested)

So, keep you inform when the new PR will be ready (in 1 or 2 days)

Cyril

@JoomliC Hint: #5078 this was a response to the issue #5066. Maybe you can incorporate the code to your implementation? The idea here to have the sidebar viewable in smaller screens

@dgt41 Thanks for that info. It would be good if @JoomliC could implement that as well. So we have a single PR to handle the sidebar. Once we have the new PR, we can close this one and #5078 to have a single point of work. Thanks guys.

Folks, don't want to hurry you but it would be good if we get this before 3.4.0 beta (release date unknown, but normally soon)

Set to "closed" on behalf of @zero-24 by The JTracker Application at issues.joomla.org/joomla-cms/4450

closing in for the new PR: #5141

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

Yes, new PR is ready : #5141

Thanks @zero-24

@dgt41 and @roland-d : #5078 is not anymore needed in new PR, as use of media query to keep sidebar as it is on 3.3.6 on small devices ;-)

@JoomliC

@dgt41 and @roland-d : #5078 is not anymore needed in new PR, as use of media query to keep sidebar as it is on 3.3.6 on small devices ;-)

Closed Thanks