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

Show in-context API documentation on details pages #2926

Merged
merged 1 commit into from Oct 16, 2019

Conversation

spadgett
Copy link
Member

@spadgett spadgett commented Oct 7, 2019

For hackday, I was looking at using the OpenAPI doc to show descriptions in more places in the UI. I wanted to get input on this change. This adds a DetailsItem component that makes it easy to add field level help descriptions for properties on our details pages. The nice thing about this approach is it gives accurate and current descriptions for almost all fields, although sometimes they are a little technical.

@openshift/team-ux-review @alimobrem @rhamilto

Screen Shot 2019-10-07 at 11 47 52 AM

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 7, 2019
@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Oct 7, 2019
@spadgett
Copy link
Member Author

spadgett commented Oct 7, 2019

Screen Shot 2019-10-07 at 12 40 12 PM

@andybraren
Copy link
Contributor

This is quite cool. It adds a lot of info/help icons, but the discoverability/learnability improvement of something like this seems worth it, imo.

  1. Do you know of any instances where we already provide metadata help text that could conflict with this? There may be at least one in a VM's Details for Boot Order. I think we could design something to accommodate OpenShift-specific and OpenAPI-provided descriptions in those (hopefully rare) cases, like maybe an Expansion component to reveal the API Docs.

  2. Could we enhance the API Explorer with deeper permalinks so that the breadcrumbs here could link to the relevant Explorer pages?

  3. It would be extra nice to expose the description of any new/unfamiliar CRs (like Machines or BareMetalHosts) somewhere within their Details page too. If we move forward with this patten, maybe another info icon could be placed next to "Deployment Overview"?

@spadgett
Copy link
Member Author

spadgett commented Oct 7, 2019

This is quite cool. It adds a lot of info/help icons, but the discoverability/learnability improvement of something like this seems worth it, imo.

Yeah, I struggled with this as well. If you have ideas to reduce the clutter, I'm all ears. My other thought was to make it a click or hover on the label, but that's less discoverable.

  1. Do you know of any instances where we already provide metadata help text that could conflict with this? There may be at least one in a VM's Details for Boot Order. I think we could design something to accommodate OpenShift-specific and OpenAPI-provided descriptions in those (hopefully rare) cases, like maybe an Expansion component to reveal the API Docs.

I'm not aware of any places offhand in core console. I do feel like if the API doc is insufficient we should improve the API doc instead of swapping out the description. The API doc description is displayed in other contexts as well like oc explain, the YAML editor, and Home -> Explore.

  1. Could we enhance the API Explorer with deeper permalinks so that the breadcrumbs here could link to the relevant Explorer pages?

Yes! @smarterclayton had the same feedback. I was actually restructuring the code some in this PR to make that easier.

  1. It would be extra nice to expose the description of any new/unfamiliar CRs (like Machines or BareMetalHosts) somewhere within their Details page too. If we move forward with this patten, maybe another info icon could be placed next to "Deployment Overview"?

Agree. That's simple to add.

@bmignano
Copy link

bmignano commented Oct 7, 2019

+1 to Andy's comments above, particularly linking the breadcrumbs to the Explore page. A minor note but wondering whether the info icon would be better suited here since it's more informational rather than field level help. I think it's probably also worth thinking about how this will interact with the new edit designs where the pencil moves to the right of the description field. I think one option is to have the label, then the info, then the pencil but curious other thoughts!

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2019
@spadgett spadgett changed the title [WIP] Show in-context API documentation on details pages Show in-context API documentation on details pages Oct 8, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 8, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2019
@spadgett
Copy link
Member Author

spadgett commented Oct 8, 2019

Rebased.

I think this is a reasonable place for a review if we agree it's something we want. If we want to change the icon or how the popover works later, we can do that in one place since it's a shared component. The idea of having anchors on the explore page for individual properties is great, but it's a big change that we should tackle in a follow on. We can add more popovers to individual pages in follow on PRs as well.

@rhamilto PTAL

@spadgett
Copy link
Member Author

spadgett commented Oct 9, 2019

/retest

@maryshak1996
Copy link

@spadgett I chatted with @andybraren and @matthewcarleton about this a bit and came up with a possible quick solution (but I'd be happy to talk to the PF team in the future about situations like this too)

  • Dotted underline for any/all definition list labels that will have these popups available. Note: for accessibility, user should have to CLICK on underlined text for the popover to to trigger, not just hover)
  • By default, the underline should be in color: var(--pf-c-button--m-plain--Color) -- same as icon buttons
  • When the popover is opened, the underline should be in color var(--pf-c-button--m-tertiary--Color)
    Example:
    Screen Shot 2019-10-10 at 12 02 53 PM

@maryshak1996
Copy link

@spadgett here's the visual treatments that use PF4 elements, as promised earlier:
spec_contextual-help

@spadgett
Copy link
Member Author

I updated the PR to use an underline:

apiservice-cabundle-injector · Details · OKD 2019-10-14 19-00-57

@spadgett
Copy link
Member Author

/assign @TheRealJon

@spadgett spadgett added this to the v4.3 milestone Oct 15, 2019
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2019
Copy link
Member

@TheRealJon TheRealJon left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spadgett, TheRealJon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@spadgett
Copy link
Member Author

/retest

1 similar comment
@spadgett
Copy link
Member Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants