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

Add reference documentation link to MetadataEditor #1386

Merged
merged 3 commits into from
Mar 10, 2021

Conversation

ajbozarth
Copy link
Member

Fixes #838

Adds a button to the top right of the MetadataEditor when a schema
has reference_url defined in it's uihints

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.

Fixes elyra-ai#838

Adds a button to the top right of the MetadataEditor when a schema
has `reference_url` defined in it's `uihints`
@ajbozarth ajbozarth self-assigned this Mar 8, 2021
@elyra-bot
Copy link

elyra-bot bot commented Mar 8, 2021

Thanks for making a pull request to Elyra!

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

@ajbozarth
Copy link
Member Author

I'll upload a screenshot in a bit (forgot to take one before doing clean-jupyterlab.sh)

Though this would be fine to merge as is (thus not wip), I'm not 100% setting on the look and feel of the button, specifically the text and the text color. Should the text be different and should I change the text color to white or would it then be unclear that it's a link? @ptitzler your feedback on those items would be most appreciated.

@ajbozarth
Copy link
Member Author

Screenshots

Airflow light mode
Screen Shot 2021-03-08 at 4 20 31 PM

kfp dark mode
Screen Shot 2021-03-08 at 4 20 41 PM

Code Snippet dark mode
Screen Shot 2021-03-08 at 4 20 48 PM

Runtime images light mode with mouse over
Screen Shot 2021-03-08 at 4 21 03 PM

airflow dark mode with mouse over
Screen Shot 2021-03-08 at 4 21 17 PM

@ptitzler
Copy link
Member

ptitzler commented Mar 9, 2021

  • Works as expected
  • Light theme: good
  • Labels: Perhaps we should add a text property to the schema that used used as label if it is defined? The current approach that concatenated the title property and the static string 'Documentation' does sound a bit odd to me due to the plural. e.g. "Runtime images documentation". Something like "Learn about STATIC_STRING" might work better but also leads to a translation problem.
  • Dark theme: the blue (?) font color of the doc link doesn't really provide a good contrast. It's much better when hovering over the link.

@ajbozarth
Copy link
Member Author

Dark theme: the blue (?) font color of the doc link doesn't really provide a good contrast. It's much better when hovering over the link.

I was going to argue that this was the color of all link text in lab, but upon checking I found that there was a css var for link text color we weren't using, so I updated both the editor and widget to use the lighter shade of blue, screenshots below:

Screen Shot 2021-03-08 at 5 50 14 PM

Screen Shot 2021-03-08 at 5 50 20 PM

@lresende
Copy link
Member

lresende commented Mar 9, 2021

Also, do we really want to have "ALL UPPERCASE" link content?
image

@ajbozarth
Copy link
Member Author

ajbozarth commented Mar 9, 2021

@lresende how does this look instead? I switch from a Button to a Link (this was actually my original implementation)

Note: I haven't pushed this change yet

Screenshots of light/dark w&w/o mouseover
Screen Shot 2021-03-09 at 10 01 23 AM
Screen Shot 2021-03-09 at 10 01 27 AM
Screen Shot 2021-03-09 at 10 01 35 AM
Screen Shot 2021-03-09 at 10 01 38 AM

@ptitzler
Copy link
Member

ptitzler commented Mar 9, 2021

Food for thought - maybe make this part of the "description" to take into consideration how LTR consumers scan a page (Top left to bottom right)

All fields marked with an asterisk are required. [Learn more ...]

This way the critical information is all in one place.

@ajbozarth
Copy link
Member Author

This way the critical information is all in one place.

I like this, let me try that and make some new screenshots

@ajbozarth
Copy link
Member Author

@ptitzler is this what you were thinking?

Screen Shot 2021-03-09 at 12 07 20 PM

@ptitzler
Copy link
Member

ptitzler commented Mar 9, 2021

Yep! I think this is much cleaner. Even though the dark theme is still not my cup of tea 😉

@ajbozarth
Copy link
Member Author

I've pushed the changes as seen in my previous commit

Copy link
Member

@ptitzler ptitzler 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 code snippets, runtime configuration, and runtime image in light and dark theme

@lresende lresende merged commit d0c2ca2 into elyra-ai:master Mar 10, 2021
@lresende lresende added this to the 2.1.0 milestone Mar 11, 2021
@ajbozarth ajbozarth deleted the doclink branch March 16, 2021 20:15
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.

Metadata editor: allow for links to "documentation" to help users understand the purpose
3 participants