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

Restore the functionality of JToolBarHelper::spacer('50px'); #11927

Closed
wants to merge 2 commits into from

Conversation

zero-24
Copy link
Member

@zero-24 zero-24 commented Sep 4, 2016

Pull Request for Issue found in the german forum https://forum.joomla.de/index.php/Thread/2505-Abstand-zwischen-task-buttons

Summary of Changes

This restores the seperator / spacer functionality this is broken since arround 2014

Testing Instructions

image

Documentation Changes Required

None

@brianteeman
Copy link
Contributor

brianteeman commented Sep 4, 2016 via email

@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on deca964

Applied the patch - no change


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

@zero-24
Copy link
Member Author

zero-24 commented Sep 4, 2016

Why would we expect that?

As we add the code that at there is a spacer with 50px ;) Add this line JToolBarHelper::spacer('50px');

@brianteeman
Copy link
Contributor

i think i misunderstand something

On 4 September 2016 at 18:51, Brian Teeman brian@teeman.net wrote:

we would expect a space of 50px between the edit and the publish button

Why would we expect that?

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@brianteeman
Copy link
Contributor

It was removed here #1686 by @rdeutz

@zero-24
Copy link
Member Author

zero-24 commented Sep 4, 2016

Correct. But i don't see a reason that this is removed. But if it should be still removed than we should deprecte that and not pointing it to a clear layout that makes no sense.

At least ist works for me with that patch so way not restore that was removed 2014?

@brianteeman
Copy link
Contributor

Your patch worked for me as well. But I'm sure that @rdeutz must have had a
good reason - even if we cant guess it

On 4 September 2016 at 19:53, zero-24 notifications@github.com wrote:

Correct. But i don't see a reason that this is removed. But if it should
be still removed than we should deprecte that and not pointing it to a
clear layout that makes no sense.

At least ist works for me with that patch so way not restore that was
removed 2014?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#11927 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8c6wAJtoCewXpDL4ZXsOPAFaZK_Nks5qmxOXgaJpZM4J0li8
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@dgrammatiko
Copy link
Contributor

Another reason might be: styling should be done (preferably) using classes not inline

@ghost
Copy link

ghost commented Sep 4, 2016

@zero-24
Thanks for this PR.

Confusing and senseless at the moment:
JToolBarHelper::spacer()

calls class

JToolbarButtonSeparator on its way through Joomla that uses

$layout = new JLayoutFile('joomla.toolbar.separator');

that is empty.

I don't know if it's better to add a new method JToolBarHelper::separator() or a new JLayout spacer.php or something. At the moment it's a completely confusing mixture of names and not obvious which JLayout one can use for overrides.

And no, I wouldn't remove Spacer/Separator or deprecate it.

@zero-24
Copy link
Member Author

zero-24 commented Sep 4, 2016

@dgt41 I'm no CSS expert do we have a way to dynamic style via CSS from PHP? In a goog and clean way ;)

Thanks @bertmert what do you suggest maybe we can proxy both methods to one layout or should they do different things?

@dgrammatiko
Copy link
Contributor

None that I know. But to be fair here joomla is using inline css all over the place so one more won't be the end of the world. Although modern design dictates that styling should be done with classes. So no real objection, just a note 😄

@ThomasFinnern
Copy link

ThomasFinnern commented Sep 5, 2016

"JToolBarHelper::spacer" is not used inside of standard joomla code. On the other side
"JToolbarHelper::divider()" is used. Both are connected as both are actual 3.6.2 not inserting space between toolbar buttons.
Fortunatelly this patch fixes both. I added the lines manually and found both working
Thanks for the fix

@rdeutz
Copy link
Contributor

rdeutz commented Sep 5, 2016

I removed it because you can't use it this way, it opens and close a "btn-group" and that doesn't makes sense. There must be something within the group. Further more it isn't a separator then.

@zero-24
Copy link
Member Author

zero-24 commented Sep 5, 2016

So we should close here and mark that methods as deprected?

@rdeutz
Copy link
Contributor

rdeutz commented Sep 5, 2016

close yes, but there might be a css framework that supports a thing like separator it the future, who knows ;-)

@zero-24
Copy link
Member Author

zero-24 commented Sep 5, 2016

Closing as requested

@zero-24 zero-24 closed this Sep 5, 2016
@zero-24 zero-24 deleted the patch-16 branch September 5, 2016 19:18
@ThomasFinnern
Copy link

Hey, for five minutes "JToolBarHelper::spacer" and "JToolbarHelper::divider()" were working in the future.

JToolbarHelper::divider() is mentioned in the component development
https://docs.joomla.org/J3.x:Developing_an_MVC_Component/Adding_ACL
It is used in 125 files of joomla. Shouldn't we give it a function ?

@rdeutz
Copy link
Contributor

rdeutz commented Sep 5, 2016

that comes from the 2.5 days, but since we are using bootstrap there is not markup for this

@zero-24
Copy link
Member Author

zero-24 commented Sep 5, 2016

But what is the problem to use a dive with xxx pixels? What kind of markup do we need?

@rdeutz
Copy link
Contributor

rdeutz commented Sep 5, 2016

no problem, what I am saying is:

  • There isn't a equivalent in bootstrap for what we had in 2.5 out of the box
  • The markup I removed hasn't makes sense
<span class="spacer">|</span>

not sure what we need to add to make it accessible, that would need an expert on this area

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

6 participants