-
Notifications
You must be signed in to change notification settings - Fork 45
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
Added tooltips for context help in ping and metric fields. Fixes #293 #300
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a great start! A few things in addition to the comments:
- The tooltips sometimes go off the screen-- I think using sveltejs-tippy (which builds off the tippy library) instead of rolling our own should fix it.
- I think instead of persisting the tooltip so the user can click "Learn More", we can just make the help icons link to the documentation. This is what the old glean dictionary does and I think it works ok.
- The pull request title should specify the issue fixed (Add tooltips with contextual help for application, ping and metric fields #293). See the contributing docs.
src/pages/MetricDetail.svelte
Outdated
<td>Relevant Bugs</td> | ||
<td> | ||
Relevant Bugs | ||
<ToolTip top={true} tip={getHelpMessage('bugs')}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably makes sense to create a small component for this (in another project, I called a similar thing HelpHoverable
)
src/helplist.tooltip.js
Outdated
@@ -0,0 +1,20 @@ | |||
export default { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the right idea, but I'd probably put this file in src/data/help.js
(I don't think there's any need to specify that they have to do with tooltips specifically)
src/helplist.tooltip.js
Outdated
bugs: { | ||
text: | ||
"Required. A list of bugs (e.g. Bugzilla or GitHub) that are relevant to this metric. For example, bugs that track its original implementation or later changes to it.", | ||
more_info: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just call this link
, for simplicity
more_info: | |
link: |
It looks like sveltejs-tippy including the tippy library directly causes some minor issues. In particular, we need to import a plugin to postprocess the css, as well. This is the same as mdauner/sveltejs-tippy#117 After fixing this, I noticed the additional problem of the tippy library referencing something we don't have in a browser context ( https://github.com/mdauner/sveltejs-tippy/blob/master/package.json#L35 The following patch, if applies, makes things work again (though the styling still needs some work): https://gist.github.com/wlach/bdaab17805e0a24fdabff718d15e9a12 Do you want to experiment with applying that, and maybe the babel thing I mention? |
Ok, I see! I could fix that now that it's clear, however, if we style need to do some styling, Why don't we eliminate this dependency all together, the overflow issue can easily be stripped down with a few tailwind classes. |
I think the underlying tippy library does a lot of stuff, I'm a bit skeptical that the behaviour can be reproduced just with CSS. You're definitely welcome to try though! |
I checked the tipsy library again, it offers a lot (way more than we need) but the flexibility is enormous so I went with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good! I have some concerns about the implementation, hopefully they won't be too hard to address. Once we have an initial draft of this working, we can start using this in more places. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, so a few things:
I think maybe some of my feedback got a bit garbled / misunderstood. Just to reframe again (maybe a bit more concretely):
- I would rename
ToolTip.svelte
toHelpHoverable.svelte
and define the various types of behaviour we want (the info icon, etc.) inside it. - The file you renamed to
HelpHoverable.js
can just be removed (since it's just an abstraction layer over the actual data which we don't need, at least not right now).
After the above suggestions are applied, MetricDetail.svelte
should look something like this:
<script>
...
import HelpHoverable from "../components/HelpHoverable.svelte"
import HelpStrings from "../data/help";
...
</script>
...
<td>Send in Pings <HelpHoverable content={help.send_in_pings.content} link={help.send_in_pings.link} /></td>
...
6a2fed7
to
e6e840b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, just have a few last requests for changes before we land.
Signed-off-by: Fenn-CS <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just one change (which I'll make), then we can land. @Iinh -- we'll need to rebase the protocol work on top of this at some point-- the amount of tailwind styling in here is pretty minimal so hopefully merging won't be so bad.
Thanks for your persistence on this @fenn-cs! |
This PR addresses #293
Method
Notes
Preview