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

Update metadata editor to use JupyterLab's new form editor #2464

Merged
merged 76 commits into from
Jun 1, 2022

Conversation

marthacryan
Copy link
Member

@marthacryan marthacryan commented Feb 8, 2022

This PR adds the package rjsf(react JSON schema form - a renderer for showing JSON schema as a form) to the metadata editor for consistency with JupyterLab (moved to using this package in the most recent release with jupyterlab/jupyterlab#11079. Going forward, this will also handle many of the more complex cases for rendering the metadata editor and provide built-in features that will solves issues such as #2269 and #2268.

Screenshots / gifs to come

Todo:

  • Still need to address validation failing on some categories and preventing save and close
    - [ ] Update documentation Follow up PR to come
  • Clean up look and feel with CSS

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.

@marthacryan marthacryan added status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. component:metadata-editor Metadata Editor Extension labels Feb 8, 2022
@elyra-bot
Copy link

elyra-bot bot commented Feb 8, 2022

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link: Binder

@akchinSTC akchinSTC added this to the 4.0.0 milestone Feb 9, 2022
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.

a few comments on the code changes since my last review, and I'm building this to test locally atm

packages/ui-components/package.json Outdated Show resolved Hide resolved
packages/ui-components/style/formeditor.css Outdated Show resolved Hide resolved
@ajbozarth
Copy link
Member

UI/UX Review as mentioned above, all tested in Safari:

  • The mouseover description shows when mousing over both the title and input area for a filed as well as if you mouse over the description itself. This hides the title for the field below. Unless there's a reason to read it while filling out the field, or to copy/paste the descriptions contents I think a better UX would be to only show when mousing over the title. (@ptitzler option on this?) (it can be seen in the video in bullet 3)
  • Show hide icon css not quite right
    Screen Shot 2022-05-23 at 4 09 49 PM
  • Form spacing changes on initial mouseover, see video below:
Screen.Recording.2022-05-23.at.4.13.14.PM.mov
  • some escaping is going wrong here:
    Screen Shot 2022-05-23 at 4 19 03 PM
  • description tooltips are misaligned for code snippet fields, also there is no description for tags (dont know if that's intentional)
    Screen Shot 2022-05-23 at 4 20 43 PM
    Screen Shot 2022-05-23 at 4 20 47 PM
  • css for this field looks off
    Screen Shot 2022-05-23 at 4 22 13 PM
  • there's no longer button highlighting when mousing over the save button like before

That's all I found on an initial pass, I only played with it for less than an hour

@marthacryan
Copy link
Member Author

  • The mouseover description shows when mousing over both the title and input area for a filed as well as if you mouse over the description itself. This hides the title for the field below. Unless there's a reason to read it while filling out the field, or to copy/paste the descriptions contents I think a better UX would be to only show when mousing over the title. (@ptitzler option on this?) (it can be seen in the video in bullet 3)

Yeah this is the current behavior so I figured it was the UX we wanted, but I could see that being a little annoying. If @ptitzler has an opinion let us know!

  • Show hide icon css not quite right

Fixed

  • Form spacing changes on initial mouseover, see video below:

I'm not able to reproduce this? Maybe we can do a call to debug

  • some escaping is going wrong here:

Fixed

  • description tooltips are misaligned for code snippet fields, also there is no description for tags (dont know if that's intentional)

Fixed misalignment of tooltips and added tooltip to code field. I don't think tags already had that so not sure if I should add that in?

  • css for this field looks off

Fixed the spacing up a little, let me know if you have more input on the style

  • there's no longer button highlighting when mousing over the save button like before

Fixed

@ptitzler
Copy link
Member

It appears that the required annotation (*) isn't displayed for all required properties, as seen in this code snippet example. Note the discrepancy between the validation messages and the property annotations:

image

@ptitzler
Copy link
Member

In various forms the label styling is inconsistent. For example (small, regular font vs bigger, bold font):
image

@ptitzler
Copy link
Member

It appears that for list-valued properties there are two new buttons displayed (move up, move down) that appear to give the user the ability to reorder list items.

image

Is this something that could be disabled? While there might be general use cases where order matters, I can't think of one for any of our properties.

Part of the problem is that the list-specific action buttons are rendered in a way that feels too attention grabbing, considering that they don't serve a practical purpose.

image

The old/existing rendering (shown here for a list-valued pipeline property) of list-specific blends in nicely and doesn't require additional screen real estate:

image

@ptitzler
Copy link
Member

Ageneral observation that the current styling appears to use almost always bold fonts (section headers, property labels, ...) as shown here for runtime configurations:

image

As a result everything seems to blend in, eliminating the benefits, for example, of section headers that were meant to help the user put labels into proper context. For comparison, here's the current rendering, which is a lot less attention grabbing:

image

@marthacryan
Copy link
Member Author

marthacryan commented May 26, 2022

Thanks @ptitzler! think I addressed all of those points. I also added those two password fields that hadn't been updated to be detected yet so all the password fields should be working now. I also added the tooltips over an info icon:
image

This tooltip shows up when the mouse is hovering over the "?" icon

Here's the updated look for the airflow runtime editor:
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.

This LGTM, I gave @marthacryan a couple UI tweaks to follow up on over a Webex that I would consider non-blocking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:metadata-editor Metadata Editor Extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants