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

In-app help content #1024

Merged
merged 7 commits into from
Feb 3, 2022
Merged

In-app help content #1024

merged 7 commits into from
Feb 3, 2022

Conversation

seancolsen
Copy link
Contributor

@seancolsen seancolsen commented Jan 25, 2022

Fixes #914

Goals

  • Add a Help component that shows help content in a popover.

    Use is as follows:

    This is text on the page. <Help>This is help content.</Help>
  • Establish a convention of placing help content inside dedicated components which live in __help__ directories close to the code where they get used. This pattern helps to separate concerns of content from concerns of functionality, keeping the higher-level components less-cluttered.

  • Move some existing help content from the Schemas page into a Help popover.

  • Add some new help content to the Table Constraints modal.

Demo

The following gif shows all the in-app help content I've added in this PR.

Peek 2022-01-25 11-59

Issues

  • In Help.scss, I had to use a couple awkward CSS selectors, button.dropdown.trigger.no-arrow.help-trigger and .dropdown.help-content because I was fighting against the specificity of the CSS within the lower-level components. @pavish I'm curious to hear your recommendation for a better way to handle this. If it were all up to me, I would be setting up our CSS very differently, so I'd like some guidance here. And I want to make sure not to use !important anywhere in our component library. With my padding: 0; CSS as an example, what's the correct way to do this? I tried setting triggerClass="padding-zero" on Dropdown in Help.scss, but that didn't work because button.btn.padding-zero has lower specificity than button.dropdown.trigger.no-arrow. I feel like we need to make some modifications to Button and/or Dropdown to support my use-case of a dropdown without padding, but I'm not sure how to approach it and stay consistent with the our CSS patterns.

  • Line height within help content is noticeably narrow. That should be addressed in Improve line-height CSS #999

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the master branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


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 open source license
    indicated in the file; 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.

Use translucent background so that it looks good against non-white
backgrounds.
Don't reference @mathesar-component-library alias because it will
result in circular references that lead to test failures.
@seancolsen seancolsen requested a review from a team January 25, 2022 17:08
@github-actions github-actions bot requested review from dhruvkb, dmos62, ghislaineguerin, kgodey, mathemancer, pavish, silentninja and zackkrida and removed request for a team January 25, 2022 17:08
@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2022

Codecov Report

Merging #1024 (51e0f64) into 531_multi_col_constraints (a0dce29) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                    Coverage Diff                     @@
##           531_multi_col_constraints    #1024   +/-   ##
==========================================================
  Coverage                      93.27%   93.27%           
==========================================================
  Files                             95       95           
  Lines                           3463     3463           
==========================================================
  Hits                            3230     3230           
  Misses                           233      233           
Flag Coverage Δ
pytest-backend 93.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0dce29...51e0f64. Read the comment docs.

@seancolsen
Copy link
Contributor Author

seancolsen commented Jan 25, 2022

@kgodey From a product perspective, would you like to give an opinion on this new pattern of sprinkling help content popovers throughout the UI?

@ghislaineguerin From a design perspective, would you like to give an opinion on the appearance and functionality of the help messages?

@kgodey
Copy link
Contributor

kgodey commented Jan 25, 2022

@seancolsen I think it's a great pattern!

@ghislaineguerin
Copy link
Contributor

@seancolsen I'm wondering if we need to have some rules for using the pop-over instead of inline text. This pattern looks perfect for definitions, but it seemed a bit lengthy to explain more in-depth concepts when adding the unique constraint. Should we separate hints from definitions maybe? For now, I think we can go ahead and have both under this pattern, and later we might decide to treat hints differently or link to the relevant documentation for longer texts.

@seancolsen
Copy link
Contributor Author

seancolsen commented Jan 25, 2022

@ghislaineguerin

link to the relevant documentation for longer texts

Yeah, linking to external docs is exactly what I'd like to do once we have them set up. For now, this in-app pattern will help users and give us a head start on the content we want to publish externally.

@silentninja
Copy link
Contributor

silentninja commented Jan 26, 2022

I like this, was aiming for a similar approach with error handling, where we could have hints to resolve the error and links to the doc that would explain in detail the cause of error(Null violation exception for example)

@seancolsen seancolsen added the pr-status: review A PR awaiting review label Jan 26, 2022
@pavish pavish self-assigned this Jan 28, 2022
@pavish
Copy link
Member

pavish commented Feb 2, 2022

@seancolsen Please feel free to update the css and dom structuring of our Dropdown and Button components. Our CSS patterns have diverged from when we initially started and these components were some of the earliest ones. I'll leave it to you to play around and update the components.

Here are a few ideas:

  • It's better to keep padding-zero as default for the Dropdown trigger i.e in the composed Button component.
  • We could set the padding to the slot content instead, for both label and arrow.
  • Arrow is currently absolutely positioned. Since the entire Button component is inline-flexed, we could remove absolute positioning for the arrow.
  • The initial plan was to avoid utility classes as much as possible, but we may still need to use them in cases like these. I would suggest to move the padding utility classes to our common css file, and remove any and all padding specifications in both Button and Dropdown scss files.

@pavish
Copy link
Member

pavish commented Feb 2, 2022

@ghislaineguerin @kgodey @seancolsen

Regarding the UX for the help content, it is currently displayed only upon clicking the help icon. There are two widely used patterns for inline-help:

  1. Show the help content on hovering upon the icon, like a tooltip. Hide it when we no longer hover upon the icon.
    • This is generally preferred when the help content does not have links or any form of user interaction.
  2. Show the help content upon clicking the icon, like a popup. Hide it when we click elsewhere, similar to how we interact with a dropdown.
    • This is preferred when the help content has links or any form of user interaction within it.

We have gone with option 2 in this PR, which I also think is best suited for us.

I'd like to confirm this from a UX perspective and how we envision inline-helps to function in the future.

@pavish pavish added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Feb 2, 2022
@kgodey
Copy link
Contributor

kgodey commented Feb 2, 2022

I think option 2 is fine.

@seancolsen
Copy link
Contributor Author

@pavish regarding the CSS.... It sounds like we're in agreement that the code in this PR works, but accentuates the need for some lower-level CSS refactoring.

This situation reminds me of #915 in which I critiqued your use of !important and you responded by opening a new ticket to address it later.

With our desire to ship features faster, I suggest taking a similar approach here. If you're okay with merging as-is, I'll create a new ticket for the CSS refactoring.

@seancolsen seancolsen removed their assignment Feb 2, 2022
@seancolsen seancolsen added pr-status: review A PR awaiting review and removed pr-status: revision A PR awaiting follow-up work from its author after review labels Feb 2, 2022
Base automatically changed from 531_multi_col_constraints to master February 3, 2022 03:21
@pavish
Copy link
Member

pavish commented Feb 3, 2022

@seancolsen Sounds good.

Please create a new issue for streamlining css, and remove the css update part of #971 and place it in the new issue.

Copy link
Member

@pavish pavish left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@seancolsen
Copy link
Contributor Author

seancolsen commented Feb 3, 2022

@pavish

Please create a new issue for streamlining css, and remove the css update part of #971 and place it in the new issue.

I added two new issues:

I kept the issues separate because I view them as independent (though of a similar topic).

With our agreed intent to de-prioritize the component library I've placed both issues in the "Future Consideration" milestone so as to avoid concerning ourselves with them before the Alpha release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: review A PR awaiting review
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Create components for building in-app documentation and help content
6 participants