-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implement tooltips #738
base: main
Are you sure you want to change the base?
Implement tooltips #738
Conversation
} | ||
|
||
.tooltip-shift-right { | ||
left: 15px !important; |
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 tweaked a great many things to get the Bootstrap tooltip to render directly atop the icon element, but it kept wanting to be 15px to the left. This shifts it back where we want it, I think. I'm open to alternatives, of course.
<%= form.label field_name, label_text, class: classes %> | ||
<%= form.label field_name, label_text, class: classes %> | ||
<% if render_tooltip? %> | ||
<%= helpers.info_icon(fill: true, classes: 'px-3 tooltip-info', data: { bs_custom_class: 'tooltip-shift-right', bs_toggle: 'tooltip', bs_title: tooltip, bs_trigger: 'click focus', tooltips_target: 'icon' }) %> |
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.
Componentize so that can be used elsewhere?
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.
@justinlittman I might hold off until I see what the other uses look like, deferring another level of abstraction until I'm sure it's valuable. As it is now, I had to touch three levels of components to get this in place. Having to touch four levels to get a tiny snippet of HTML where it belongs makes me wonder about the cost/benefit ratio of our current component architecture (at least the ones in app/components/elements/
---these context-free components seem to "smell" worse than the contextful ones). It's a whole lot of hierarchy chock full of components, a bunch of which smell like junk drawers (in terms of how many parameters they care about and how many they merely pass on to lower-level components). I understand the desire to reduce repetition in code, but I've also become wary of refactors that optimize for DRYing code out for fear of being premature abstractions. Maybe more a retro topic more than a code review thing. cc: @aaron-collier @lwrubel @thatbudakguy
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 was under the assumption that we had tooltips all over the place, not just in form labels. If that is mistaken, then agree that no need to componentize.
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.
Other than the collection and work forms, there are also tool tips in the help form. See #670
f6ea4ba
to
ecd39c1
Compare
Fixes #450
ecd39c1
to
65281ad
Compare
Fixes #450