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

Two new features: #10

Closed
wants to merge 1 commit into from
Closed

Two new features: #10

wants to merge 1 commit into from

Conversation

dmcritchie
Copy link

Bonjour Jean-Marc,

I've added a couple of features to ol3-ext that I needed to make use of your control bar in my mapping application, as described below. I am providing them here in case they should be of interest to you:

  1. Recursively deactivate any controls in sub control bar when parent control is deactivated. When sub control bar becomes invisible because parent control has been deactivated, I need for any previously active control(s) in that sub control bar to be automatically deactivated as well.

  2. New defaultActive parameter for Toggle control. For top-level control bar, same as 'active' param. For nested control bar, sets the control active when parent control is active. Handy if user typically needs to start with a specific control in a main or sub control bar.

Item (1) seems similar to something you apparently started working on, but had commented out.

Feel free to ignore or change any of this, of course.

Once I can make controls in sub control bars change their background color when activated (issue #8), I'll be all set to go, I believe.

Thanks for a very helpful tool set.

Best,
Dennis

1) Recursively deactivate any controls in sub control bar when parent control is deactivated.
2) New defaultActive parameter for Toggle control. For top-level control bar, same as 'active' param. For nested control bar, sets the control active if parent control is active.
@Viglino
Copy link
Owner

Viglino commented Nov 1, 2016

Hello Dennis,

For the first point, I'm not sure it should be the default behavioured as it is application dependant (some may want the sub controls to stay active, specially if they are options of the main control).
I've added a useful function to deactivate all controls in a bar. So it's easy for the application to do that when necessary. It is used in the example http://viglino.github.io/ol3-ext/examples/map.control.bar.html

edit2.on("change:active", function(e)
{   if (!e.active) subBar.deactivateControls();
});

If it should be set, it would be optional to let the user choose the behaviour.

The second issue is more interesting to set a control active the first time it is shown (an autoactivate option). Thus the control will auto activate when it is inserted the first time in a bar or when the top level control is activated if in a sub bar.

I'll try to think about it.

Best,

@dmcritchie
Copy link
Author

Thanks for reply, Jean-Marc.

I understand your point that the deactivation behavior should not be the default since you know of use cases where sub bar controls should stay active (I was not sure), and I see the example you mention below. However, I see a couple of problems with the way you have it implemented:

  1.  It requires that the control bar user know internal details of how the Toggle and Bar classes work. I find it better practice if the desired behavior can be controlled from the interface without having to know the internals.
    
  2.  The coding approach you suggest is not recursive. So if I had 2 levels of nested control bars, I would want them all to be deactivated when the master control was deactivated. With your approach, this would require additional coding.
    

Instead, I would recommend adding a parameter to the Bar class (perhaps “autoDeactivate”), which if present would cause all controls in that control bar to be deactivated when the parent control is deactivated. By setting this parameter in all sub control bars, the deactivation would be recursive, and no knowledge of the class internals would be required.

If you think this is a good idea, I am happy to add it to my code, commit it, and send you another pull request.

Let me know how you’d like to proceed. I do need these features, but if they are not to your liking, we can certainly keep the code divergent.

Best,

Dennis

@Viglino
Copy link
Owner

Viglino commented Nov 4, 2016

Hi Dennis

I've taken your idea into account in the last commit (2de69e6).
I've added an autoDeactivate option to deactivate all controls in a subbar when the parent control is deactivated.

I've added an autoActive option at the ol.contorl.Toggle to make it active when inserted in a subbar when the parent control is activated. If it has a subbar it is displayed as well.

I've also changed the css to make subbars display correctly as you can see in the example with the "pencil control": http://viglino.github.io/ol3-ext/examples/map.control.bar.html

Best regards,

@dmcritchie
Copy link
Author

Merci beaucoup, Jean-Marc!

It works beautifully! I will be able to make very good use of these ol3-ext classes.

Tes efforts sont très appréciés.

Dennis

@Viglino
Copy link
Owner

Viglino commented Nov 6, 2016

Hi Dennis,

I've made some changes in the ol.control.Toggle. Now it derives from an ol.control.Button class (this means you have to add the ol.control.Button.js to your pages). The ol.control.Button is a simple push button with no state.

I also plan some refactoring and will remove the subbar from the ol.control.Bar.addControl() function to put it in the ol.control.Toggle that activates it. It will be more consistent as you can"t add a subbar on other controls. I'll tell you when this will be done.

@+

@dmcritchie
Copy link
Author

Hi Jean-Marc,

Thanks for letting me know. I look forward to your refactoring changes as well.

Best,
Dennis

@dmcritchie
Copy link
Author

Hi Jean-Marc,

I noticed one missing behavior with your changes: addControl does not properly autoActivate bar controls, whether I use the autoActivate or "active" parameter. There is no code to support the former within addControl, and while "active" on a top-level bar control does cause the level-1 sub control bar to be visible, it does not cause the autoActivate logic in onActivateControl_() to be executed. So none of the level-1 sub bar controls are activated, and the 2nd-level sub bar remains invisible.

I had to re-add the following code to enable the desired behavior:

// If autoActivate control,
if (c.get('autoActivate') == true) {
    // if top-level control bar, set control active.
    if (!this.pcontrol) {
        c.setActive(true);
    }
    // If nested control, set control active if parent control is active.
    else {
        if (this.pcontrol.getActive())
            c.setActive(true);
    }

"pcontrol" was defined earlier as shown here:

if (bar)
{   this.controls_.push(bar);
    bar.setTarget(c.element);
    $(bar.element).addClass("ol-option-bar");
    c.subBar_ = bar;
    bar.pcontrol = c;
}

Otherwise, it all works great!

Thanks again.

Dennis

@Viglino
Copy link
Owner

Viglino commented Nov 9, 2016

Hello,
Yes it's a drawback of the method to affect the subbar: the toggle button that controls the subbar doesn't know it. I plan to change it and set the subbar in a Toggle control. Thus activating/deactivating the control will automatically interact with the subbar it controls.
It will be solved in the next commit...
@+

@dmcritchie
Copy link
Author

Great!

Thanks,
Dennis

@Viglino
Copy link
Owner

Viglino commented Nov 13, 2016

Hi,
I've committed a new version.
Inserting a subbar is no longer in the ol.control.Bar.addControl() function but in the ol.control.Toggle that activates it.
Instead of:

var c = new ol.control.Toggle ();
bar.addControl (c, subbar);

use:

var c = new ol.control.Toggle ({ bar: subbar });
bar.addControl (c);

@+

@Viglino Viglino closed this Nov 13, 2016
@dmcritchie
Copy link
Author

Works very well! Thanks for all your help, Jean-Marc.

Dennis

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.

2 participants