Skip to content

Conversation

@Carreau
Copy link
Member

@Carreau Carreau commented Aug 25, 2015

Mostly for now to be sure that for all menu item there is
a corresponding action.

So that some menu actions are available from command palette.
We can work on auto-generating menus after.

Note to myself. AVAILABLE ONLY HAVE ONE L, get that in your head.

@ellisonbg
Copy link
Contributor

Pinging @sccolbert and @dwillmer on this. Chris and David - here is the work Matthias has been doing to start to use the existing action system in our menus.

@Carreau
Copy link
Member Author

Carreau commented Aug 26, 2015

Note, this is a rewriting from scratch of ipython/ipython#7285
there is no technical challenges here, it's just using one extra level of abstraction instead of direct callback binding, just to make sure all actions we use in menus exists.

@Carreau Carreau closed this Aug 28, 2015
@Carreau Carreau reopened this Sep 6, 2015
@minrk
Copy link
Member

minrk commented Sep 7, 2015

👍 to this. I like using the actions stuff (except the help-index part). I'm happy to rebase #400 on this one after I test it a bit.

@Carreau
Copy link
Member Author

Carreau commented Sep 14, 2015

Rebased, not tested.

@Carreau
Copy link
Member Author

Carreau commented Sep 14, 2015

Manually tested, seem to work.

Ping @minrk who worked on these file recently

@Carreau
Copy link
Member Author

Carreau commented Sep 14, 2015

Rebased.

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, had to lookup IIFE acronym - also, probably don't want "F-you" in the comment, even though I totally agree, it's super annoying!

Copy link
Member Author

Choose a reason for hiding this comment

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

Wha't the problem with Frickin-you Javascript ?

@jdfreder
Copy link
Contributor

I noticed you left a few of the menu things as non-actions. Would you want to go ahead and convert them all to actions in this PR? Or in a later PR?

@Carreau
Copy link
Member Author

Carreau commented Sep 15, 2015

Or in a later PR?

Later PR easier to test manually in an exaustive manner at every rebase.

And deprecate private, unused thing.
@Carreau
Copy link
Member Author

Carreau commented Sep 17, 2015

Rebased once more. @jdfreder ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I now pass event in here.

Copy link
Member

Choose a reason for hiding this comment

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

for-in always needs Object.hasOwnProperty because js is the best.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think Object.keys(id_actions_dict) works, too.

@jdfreder
Copy link
Contributor

This is looking great, after you address Min's comments - I think this is good to go!

@Carreau
Copy link
Member Author

Carreau commented Sep 18, 2015

Done:

-        for(var idx in id_actions_dict){
+        var keys = Object.keys(id_actions_dict);
+        for(var i in keys){
+            var idx = keys[i];

Copy link
Member

Choose a reason for hiding this comment

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

Sorry. for(...in) is just never safe. It might as well not exist. You have to use

Object.keys(obj).map(function (key) {...});

or filter on obj.hasOwnProperty(key).

some illustration

@Carreau
Copy link
Member Author

Carreau commented Sep 18, 2015

        for(var idx in id_actions_dict){
+            if (!id_actions_dict.hasOwnProperty(idx)){
+                continue;
+            }
             var id_act = id_actions_dict[idx];

Better ?

minrk added a commit that referenced this pull request Sep 19, 2015
Working on using actions for menu.
@minrk minrk merged commit ebe1a7c into jupyter:master Sep 19, 2015
@minrk
Copy link
Member

minrk commented Sep 19, 2015

@Carreau yup. I apologize on behalf of Javascript.

@Carreau
Copy link
Member Author

Carreau commented Sep 19, 2015

I apologize on behalf of Javascript.

No need to.

Thanks !

@Carreau Carreau deleted the action-menu branch October 22, 2015 17:05
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 552a5a0 on Carreau:action-menu into ** on jupyter:master**.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants