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

Create a button in palette for all schema #906

Merged
merged 6 commits into from
Sep 4, 2020

Conversation

marthacryan
Copy link
Member

Automatically detects all schema and creates a button to open the widget for a given schema.
image

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.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@kevin-bates
Copy link
Member

Hmm. The plurality is off. Since namespace names are plural and schema names are singular, I wonder if the namespace name is what is wanted here anyway. Since 2 of the 3 are single-schema namespaces, they just work. The other would should KFP and AIrflow "runtimes" leaving the headings with...

Show Code Snippets
Show Runtime Images
Show Runtimes

Actually, the namespace names are lowercase and values like "code-snippets", "runtime-images" and "runtimes". So this is another great example of promoting namespaces to full objects where they too could have display_name values - leading to what I showed above (with proper case and spacing). Until then, I don't think this is going to fly, unfortunately.

@marthacryan
Copy link
Member Author

@kevin-bates What if we add 'Widget' after each of them? So "Show Code Snippet Widget" or "Show Runtime Image Widget"

@kevin-bates
Copy link
Member

To show a "widget" sounds odd to me.

Also, I think we should forgo using display_name from the schema because its more of an application property defined in the schema. Instead, we added title everywhere and that's what should be used here instead. When dealing with metadata instances you would reference display_name.

@marthacryan
Copy link
Member Author

@kevin-bates Yeah good point - maybe "browser"?

@lresende lresende requested a review from kevin-bates September 1, 2020 23:55
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

I just have the one comment about using title over display_name when dealing with the schema file. We should remove the top-level display_name property.

My bigger issue is with the plurality of these section headers as noted in the conversation. If we're going to inject a top-level uihints, then that might as well contain the full widget title(?):

Show Runtime Images

Copy link
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

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

Only managed a partial review, but I agree with what @kevin-bates has said so far

@ptitzler
Copy link
Member

ptitzler commented Sep 2, 2020

"Manage ..." would probably be more accurate because the metadata widget exposes the entire set of CRUD operations, not just R(ead)/Show.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@marthacryan
Copy link
Member Author

How's this look? I made it so that it uses the title by default but if there's a uihints.title it'll use that instead.
image

Copy link
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

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

Didn't test locally, but code lorn

Copy link
Member

@karlaspuldaro karlaspuldaro left a comment

Choose a reason for hiding this comment

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

LGTM :)

@marthacryan
Copy link
Member Author

Ok while working with this I figured it might be good to also implement that "Close tab" ability for metadata viewers in the left side pane since they can really add up and now it's easy to reopen any of them. Just pushed those changes but if you don't think it's good with this PR I can undo that. Thoughts?

@marthacryan
Copy link
Member Author

Just added a small change so that it won't show the "Close tab" option for code snippets or runtimes (because they're supposed to be more permanent). It was causing a small bug of creating a generic metadata widget for code snippets (which doesn't include the coding languages in the display and doesn't include the insert / copy buttons).

@marthacryan
Copy link
Member Author

So here's what the context menu looks like for code snippets / runtimes:
image

And here's the context menu for any other metadata schema present:
image

Copy link
Member

@karlaspuldaro karlaspuldaro left a comment

Choose a reason for hiding this comment

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

Nice adding a close tab option. Re-ran this locally and looks good.

Copy link
Member

@lresende lresende left a comment

Choose a reason for hiding this comment

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

LGTM - Tested locally and all looks good.

@lresende lresende merged this pull request into elyra-ai:master Sep 4, 2020
lresende pushed a commit that referenced this pull request Sep 4, 2020
Add new commands to the command palette to enable
opening existing metadata UI for all available schemas.
@lresende lresende added this to the 1.2.0 milestone Sep 16, 2020
@marthacryan marthacryan deleted the show-metadata branch April 23, 2021 15:40
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