Elyra launcher #782

lresende · 2020-07-27T15:52:05Z

Use our own Launcher implementation to properly order Elyra category/section and display project logo

Todos:

Make this the default Launcher when Elyra is installed
Change the Elyra section logo to be project logo

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

packages/theme/src/index.ts

lresende · 2020-07-28T01:37:10Z

This seems to be working now, it would be nice to have the icon on Elyra section changed to the actual Elyra icon instead of the Pipeline editor one.

packages/theme/src/index.ts

vabarbosa · 2020-07-28T13:39:32Z

packages/theme/src/launcher.tsx

+          categories.push(cat);
+        }
+        if (cat.key === 'Elyra') {
+          // change the icon here...


the category icon is taken from the first item in a category hence the default to the pipeline editor icon.

one possible option to change the icon is to use the iconClass (property of the command) and set the image accordingly via CSS rather than try to search & replace via the children props

I am ok with that, are you volunteering to help on the css side?

i can take a look

i have pushed the icon update (35f26a2) into this PR:

vabarbosa · 2020-07-28T13:41:22Z

would it be worth adding a Code Snippet card to the Elyra category, which would launch the Add new code-snippet form?

vabarbosa · 2020-07-28T13:47:40Z

packages/theme/src/launcher.tsx

+/**
+ * The known categories of launcher items and their default ordering.
+ */
+const KNOWN_CATEGORIES = ['Notebook', 'Console', 'Elyra', 'Other'];


any reason Elyra shouldn't immediately follow the Notebook category (i.e., making it the second category)?

I thought about even disabling the Console category, but I am not sure how popular it is. The only reason they are together (Notebook and Console) is because they have a 1:1 relationship in terms of cards/tiles.

I agree Console doesn't appear very useful to me. I suppose it might provide a better "scratchpad" kind of experience. I think moving Elyra up one slot would be good - although when there are lots of kernel types (which typically won't be the case) it could look a little odd since Elyra will break the 1:1 parallelism that folks are used to. (But that shouldn't be a reason for making the Launcher more useful.)

Co-authored-by: va barbosa <vabarbosa@users.noreply.github.com>

lresende · 2020-07-28T14:26:14Z

would it be worth adding a Code Snippet card to the Elyra category, which would launch the Add new code-snippet form?

Not a bad idea, as long as the extension creates it, it will show up properly here on the launcher.

Show the Elyra icon in the Elyra section of the launcher

marthacryan

Just tried this locally and it works great! I agree about the ordering of the sections (Elyra higher than console) but other than that, LGTM.

lresende · 2020-07-28T17:30:49Z

Below is how this looks like without console, which indeed makes it much more clean, but I don't feel confortable removing it unless the user can somehow re-enable it.

vabarbosa · 2020-07-28T17:56:15Z

Below is how this looks like without console, which indeed makes it much more clean, but I don't feel confortable removing it unless the user can somehow re-enable it.

i wouldn't recommend removing Console but rather just placing it lower in the order (below Elrya).

lresende · 2020-07-28T19:02:42Z

After some discussion, looks like it's better just to move the Elyra section above Others and leave everything else as is.

ajbozarth

With the changes I just committed, this LGTM

ajbozarth · 2020-07-29T02:03:46Z

@vabarbosa I replaced your fix for the elyra icon with another solution I figured out. iconClass support was deprecated in Lab 2.0 and is set to be removed as soon as 3.0 so I would prefer we don't rely on it. As such I did some digging into react and figured out a way to replace the LabIcon itself.

lresende · 2020-07-29T02:06:09Z

We should also revisit the launcher interface on the lab side to make it more extensible I guess.

vabarbosa · 2020-07-29T04:41:01Z

@vabarbosa I replaced your fix for the elyra icon with another solution I figured out. iconClass support was deprecated in Lab 2.0 and is set to be removed as soon as 3.0 so I would prefer we don't rely on it. As such I did some digging into react and figured out a way to replace the LabIcon itself.

unless i am misunderstanding, it is icon that is being deprecated in favor of iconClass or at least that's how i interpreted these comments:

https://github.com/jupyterlab/lumino/blob/master/packages/commands/src/index.ts#L194,L229

https://github.com/jupyterlab/jupyterlab/blob/master/packages/launcher/src/index.tsx#L199,L203

are those comments incorrect? is iconClass (not icon) what is actually being deprecated? if so, i guess i can go along with the approach.

ajbozarth · 2020-07-29T04:54:22Z

@vabarbosa Ah I see where you're confused.

icon has a messy history. A while back (before I started working with jupyter) icon was deprecated in favor of iconClass which behaved the same way, being a css class string, but was clearer in its actual meaning.

But then when LabIcon was implemented for Lab 2.0 the deprecated icon was overloaded with the new functionality. So what's deprecated is the ability to set icon to a string, which would just be aliased to iconClass. The new functionality is that icon can also be a LabIcon (which extends VirtualElement.IRenderer). This way of handling icons is the new standard. You can learn more about LabIcon here: https://github.com/jupyterlab/jupyterlab/blob/5555c77a17d48b973c66cb32141844f2ce34e10f/packages/ui-components/CONTRIBUTING.md

ajbozarth · 2020-07-29T05:01:09Z

Also if you want help with understanding how the code in your second link handle all three cases (really deprecated icon as a string, recently deprecated iconClass, and new icon as a LabIcon) then I can hope on a 5min call tomorrow to explain it (it took me some time to wrap my head around what those few line were actually accomplishing.

vabarbosa · 2020-07-29T12:53:35Z

thanks for the clarification

lresende · 2020-07-29T16:44:22Z

I guess we are in consensus here. Note that there is still a need to make this area a little more flexible on core and I will follow up with that later on.

marthacryan requested changes Jul 27, 2020

View reviewed changes

packages/theme/src/index.ts Outdated Show resolved Hide resolved

lresende marked this pull request as draft July 28, 2020 00:02

lresende added 5 commits July 27, 2020 18:34

Enable custom JupyterLab Launcher for Elyra

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

fc7471b

Small updates to complile and run

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

a8c8869

Replace logic

f913a42

Fix lint

ded13c7

Disable lab launcher

116f0c5

lresende force-pushed the elyra-launcher branch from bf77823 to 116f0c5 Compare July 28, 2020 01:34

lresende marked this pull request as ready for review July 28, 2020 01:35

lresende added 2 commits July 27, 2020 20:47

Remove instrumentation code

35e5395

Remove more instrumentation code and comments

6d5f2a2

vabarbosa reviewed Jul 28, 2020

View reviewed changes

packages/theme/src/index.ts Outdated Show resolved Hide resolved

vabarbosa reviewed Jul 28, 2020

View reviewed changes

Update packages/theme/src/index.ts

1c390d1

Co-authored-by: va barbosa <vabarbosa@users.noreply.github.com>

lresende added the status:Work in Progress label Jul 28, 2020

Set the Elyra category icon in the launcher

35f26a2

Show the Elyra icon in the Elyra section of the launcher

lresende removed the status:Work in Progress label Jul 28, 2020

marthacryan approved these changes Jul 28, 2020

View reviewed changes

vabarbosa self-requested a review July 28, 2020 20:51

vabarbosa approved these changes Jul 28, 2020

View reviewed changes

lresende and others added 2 commits July 28, 2020 17:56

Better naming for launcher class

9f527a2

replace icon using react rather than css

2983522

ajbozarth approved these changes Jul 29, 2020

View reviewed changes

lresende merged commit 98703b4 into elyra-ai:master Jul 29, 2020

lresende deleted the elyra-launcher branch July 29, 2020 16:48

karlaspuldaro mentioned this pull request Sep 17, 2020

Update launcher image #935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elyra launcher #782

Elyra launcher #782

lresende commented Jul 27, 2020 •

edited by vabarbosa

Loading

lresende commented Jul 28, 2020

vabarbosa Jul 28, 2020

lresende Jul 28, 2020

vabarbosa Jul 28, 2020

vabarbosa Jul 28, 2020

vabarbosa commented Jul 28, 2020

vabarbosa Jul 28, 2020 •

edited

Loading

lresende Jul 28, 2020

kevin-bates Jul 28, 2020

lresende commented Jul 28, 2020

marthacryan left a comment

lresende commented Jul 28, 2020

vabarbosa commented Jul 28, 2020 •

edited

Loading

lresende commented Jul 28, 2020

ajbozarth left a comment

ajbozarth commented Jul 29, 2020

lresende commented Jul 29, 2020 •

edited

Loading

vabarbosa commented Jul 29, 2020 •

edited

Loading

ajbozarth commented Jul 29, 2020

ajbozarth commented Jul 29, 2020

vabarbosa commented Jul 29, 2020

lresende commented Jul 29, 2020

Elyra launcher #782

Elyra launcher #782

Conversation

lresende commented Jul 27, 2020 • edited by vabarbosa Loading

lresende commented Jul 28, 2020

vabarbosa Jul 28, 2020

Choose a reason for hiding this comment

lresende Jul 28, 2020

Choose a reason for hiding this comment

vabarbosa Jul 28, 2020

Choose a reason for hiding this comment

vabarbosa Jul 28, 2020

Choose a reason for hiding this comment

vabarbosa commented Jul 28, 2020

vabarbosa Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

lresende Jul 28, 2020

Choose a reason for hiding this comment

kevin-bates Jul 28, 2020

Choose a reason for hiding this comment

lresende commented Jul 28, 2020

marthacryan left a comment

Choose a reason for hiding this comment

lresende commented Jul 28, 2020

vabarbosa commented Jul 28, 2020 • edited Loading

lresende commented Jul 28, 2020

ajbozarth left a comment

Choose a reason for hiding this comment

ajbozarth commented Jul 29, 2020

lresende commented Jul 29, 2020 • edited Loading

vabarbosa commented Jul 29, 2020 • edited Loading

ajbozarth commented Jul 29, 2020

ajbozarth commented Jul 29, 2020

vabarbosa commented Jul 29, 2020

lresende commented Jul 29, 2020

lresende commented Jul 27, 2020 •

edited by vabarbosa

Loading

vabarbosa Jul 28, 2020 •

edited

Loading

vabarbosa commented Jul 28, 2020 •

edited

Loading

lresende commented Jul 29, 2020 •

edited

Loading

vabarbosa commented Jul 29, 2020 •

edited

Loading