gavin/maybe
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 28 Wiki Activity

Add stimulus tooltip controller #1065

Merged
tonyvince merged 13 commits from 1051-add-stimulus-tooltip-controller into main 2024-08-08 18:53:27 +08:00
Conversation 9 Commits 13 Files Changed 11 +194 -57
tonyvince commented 2024-08-07 01:32:54 +08:00 (Migrated from github.com)
Copy Link

Close #1051

https://github.com/user-attachments/assets/594f5a08-0591-4678-bc15-24587bc914c6

Close #1051 https://github.com/user-attachments/assets/594f5a08-0591-4678-bc15-24587bc914c6
zachgoll (Migrated from github.com) reviewed 2024-08-07 06:00:42 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Awesome work here, looks great and well organized! Just left a few comments around Popper and some of its global styling.

Awesome work here, looks great and well organized! Just left a few comments around Popper and some of its global styling.
app/assets/stylesheets/application.tailwind.css
zachgoll (Migrated from github.com) commented 2024-08-07 05:58:37 +08:00
Copy Link

Would the components layer be a better spot for this?

https://tailwindcss.com/docs/adding-custom-styles#adding-component-classes

I think we'd be okay with what you have given you're not using any Tailwind classes, but I usually throw these types of things in the components layer so we can use modifiers like hover: with the custom styles.

Would the components layer be a better spot for this? https://tailwindcss.com/docs/adding-custom-styles#adding-component-classes I _think_ we'd be okay with what you have given you're not using any Tailwind classes, but I usually throw these types of things in the components layer so we can use modifiers like `hover:` with the custom styles.
config/importmap.rb
@@ -46,3 +46,7 @@ pin "d3-zoom" # @3.0.0
pin "delaunator" # @5.0.1
zachgoll (Migrated from github.com) commented 2024-08-07 05:40:03 +08:00
Copy Link

Is there a reason you chose PopperJS over the newer @floating-ui package? I've found it to be a bit more lightweight and more predictable in some projects I've implemented it with.

https://floating-ui.com/docs/motivation

Is there a reason you chose PopperJS over the newer `@floating-ui` package? I've found it to be a bit more lightweight and more predictable in some projects I've implemented it with. https://floating-ui.com/docs/motivation
test/system/tooltips_test.rb
@@ -0,0 +22,4 @@
end
find("body").click
assert find("#tooltip", visible: false)
end
zachgoll (Migrated from github.com) commented 2024-08-07 05:59:33 +08:00
Copy Link

Nice!

Nice!
tonyvince (Migrated from github.com) reviewed 2024-08-07 20:44:26 +08:00
config/importmap.rb
@@ -46,3 +46,7 @@ pin "d3-zoom" # @3.0.0
pin "delaunator" # @5.0.1
tonyvince (Migrated from github.com) commented 2024-08-07 20:44:26 +08:00
Copy Link

Done in 158007e

Done in [158007e](https://github.com/maybe-finance/maybe/pull/1065/commits/158007e9f22c2939fbc9d50d91b3d560109abd19)
tonyvince (Migrated from github.com) reviewed 2024-08-07 20:44:46 +08:00
app/assets/stylesheets/application.tailwind.css
tonyvince (Migrated from github.com) commented 2024-08-07 20:44:46 +08:00
Copy Link

Done in 1d4ab25

Done in [1d4ab25](https://github.com/maybe-finance/maybe/pull/1065/commits/1d4ab25adc8462ef56b9cc6186d5e2e2a3b5e221)
zachgoll (Migrated from github.com) reviewed 2024-08-08 07:30:06 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Thanks for the update with the Floating UI, looks good.

Once the notes are addressed on the Stimulus controller I think we'll be good to merge.

Thanks for the update with the Floating UI, looks good. Once the notes are addressed on the Stimulus controller I think we'll be good to merge.
app/javascript/controllers/tooltip_controller.js
@@ -0,0 +1,74 @@
import { Controller } from '@hotwired/stimulus'
zachgoll (Migrated from github.com) commented 2024-08-08 07:01:30 +08:00
Copy Link

With Stimulus we can provide default values for the value targets, which would allow us to remove this method and replace the option variables with the targets.

static values = { 
  placement: { type: String, default: "top" },
  offset: { type: Number, default: 10 },
  crossAxis: { type: Number, default: 0 },
}
With Stimulus we can provide default values for the value targets, which would allow us to remove this method and replace the option variables with the targets. ```js static values = { placement: { type: String, default: "top" }, offset: { type: Number, default: 10 }, crossAxis: { type: Number, default: 0 }, } ```
zachgoll (Migrated from github.com) commented 2024-08-08 07:03:51 +08:00
Copy Link

Here and in showTooltip, we're calling tooltip as a global variable. This works because there is an element in the DOM with an id of tooltip, but creates a tight coupling between the view and this controller. We'll eventually need to support multiple tooltips on the same page, so this will break when we do that.

I think we should use this.tooltipTarget here for stability moving forward

Here and in `showTooltip`, we're calling `tooltip` as a global variable. This works because there is an element in the DOM with an id of `tooltip`, but creates a tight coupling between the view and this controller. We'll eventually need to support multiple tooltips on the same page, so this will break when we do that. I think we should use `this.tooltipTarget` here for stability moving forward
zachgoll (Migrated from github.com) commented 2024-08-08 07:13:42 +08:00
Copy Link

We can remove element from this targets array. By default, Stimulus controllers set the this.element property which we can use.

Furthermore, I think we should introduce arrow as a target here. We're heavily depending on the arrow to have an id of arrow which will have to be extracted when we start introducing multiple tooltips per page.

So the effect of these changes would be:

// Controller

static targets = ["arrow", "tooltip"];

connect() {
  this.element.addEventListener("mouseenter", this.showTooltip)
  this.element.addEventListener("mouseleave", this.hideTooltip)
  this.element.addEventListener("focus", this.showTooltip)
  this.element.addEventListener("blur", this.hideTooltip)
}

disconnect() {
  this.element.removeEventListener("mouseenter", this.showTooltip)
  this.element.removeEventListener("mouseleave", this.hideTooltip)
  this.element.removeEventListener("focus", this.showTooltip)
  this.element.removeEventListener("blur", this.hideTooltip)
}

# View

<div data-tooltip-target="arrow"></div>
We can remove `element` from this targets array. By default, Stimulus controllers [set the `this.element` property](https://stimulus.hotwired.dev/reference/controllers#properties) which we can use. Furthermore, I think we should introduce `arrow` as a target here. We're heavily depending on the arrow to have an `id` of `arrow` which will have to be extracted when we start introducing multiple tooltips per page. So the effect of these changes would be: ```js // Controller static targets = ["arrow", "tooltip"]; connect() { this.element.addEventListener("mouseenter", this.showTooltip) this.element.addEventListener("mouseleave", this.hideTooltip) this.element.addEventListener("focus", this.showTooltip) this.element.addEventListener("blur", this.hideTooltip) } disconnect() { this.element.removeEventListener("mouseenter", this.showTooltip) this.element.removeEventListener("mouseleave", this.hideTooltip) this.element.removeEventListener("focus", this.showTooltip) this.element.removeEventListener("blur", this.hideTooltip) } ``` ```erb # View <div data-tooltip-target="arrow"></div> ```
zachgoll (Migrated from github.com) commented 2024-08-08 07:28:22 +08:00
Copy Link

Think we can remove this method. When the controller disconnects it should wipe out any tooltip content.

Think we can remove this method. When the controller disconnects it should wipe out any tooltip content.
app/views/accounts/_tooltip.html.erb
@@ -0,0 +1,26 @@
<%# locals: (account:) -%>
zachgoll (Migrated from github.com) commented 2024-08-08 07:20:24 +08:00
Copy Link
  <div data-tooltip-target="arrow"></div>
```suggestion <div data-tooltip-target="arrow"></div> ```
app/views/accounts/show.html.erb
@@ -52,7 +52,12 @@
<div class="bg-white shadow-xs rounded-xl border border-alpha-black-25 rounded-lg">
zachgoll (Migrated from github.com) commented 2024-08-08 07:20:47 +08:00
Copy Link
        <div class="flex items-center gap-1">
```suggestion <div class="flex items-center gap-1"> ```
tonyvince (Migrated from github.com) reviewed 2024-08-08 15:06:16 +08:00
app/javascript/controllers/tooltip_controller.js
@@ -0,0 +1,74 @@
import { Controller } from '@hotwired/stimulus'
tonyvince (Migrated from github.com) commented 2024-08-08 15:06:16 +08:00
Copy Link

03344a2

[03344a2](https://github.com/maybe-finance/maybe/pull/1065/commits/03344a2677142d7621bd29999620f0a058420035)
tonyvince (Migrated from github.com) reviewed 2024-08-08 15:06:37 +08:00
app/javascript/controllers/tooltip_controller.js
@@ -0,0 +1,74 @@
import { Controller } from '@hotwired/stimulus'
tonyvince (Migrated from github.com) commented 2024-08-08 15:06:37 +08:00
Copy Link

62fd745

[62fd745](https://github.com/maybe-finance/maybe/pull/1065/commits/62fd7453c6e876245bb873dcd49083949e9b33ae)
tonyvince (Migrated from github.com) reviewed 2024-08-08 15:06:58 +08:00
app/javascript/controllers/tooltip_controller.js
@@ -0,0 +1,74 @@
import { Controller } from '@hotwired/stimulus'
tonyvince (Migrated from github.com) commented 2024-08-08 15:06:58 +08:00
Copy Link

79f6ab0

[79f6ab0](https://github.com/maybe-finance/maybe/pull/1065/commits/79f6ab073ce4fc71d24803896450bc98a4f6d4c9)
tonyvince (Migrated from github.com) reviewed 2024-08-08 15:12:53 +08:00
app/javascript/controllers/tooltip_controller.js
@@ -0,0 +1,74 @@
import { Controller } from '@hotwired/stimulus'
tonyvince (Migrated from github.com) commented 2024-08-08 15:12:53 +08:00
Copy Link

32ed87a

[32ed87a](https://github.com/maybe-finance/maybe/pull/1065/commits/32ed87a1ec0fbc4bd74a77dda631625f7dceb3fe)
zachgoll (Migrated from github.com) approved these changes 2024-08-08 18:53:17 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Awesome!

Awesome!
Sign in to join this conversation.
Reviewers
No Reviewers
tonyvince
zachgoll
Labels
Clear labels
📆 Planned
Not ready to be worked on yet, but planned for future
 💻 Self Hosted only
Issues pertaining to self-hosted versions of Maybe
 ⚙️ Config
ℹ️ Needs info
Further information is requested
 📱 Mobile
Bugs exclusively happening on mobile devices
 1️⃣ High Priority
High priority issues will be worked on next
 🚨 Urgent
Issues that are urgent for app functionality
 3️⃣ Low priority
Contributions accepted, but Maybe team will not be working on this in the near term
 2️⃣ Medium Priority
Community contributions accepted, Maybe team only works on if there are no high priority items open
 ⚡ Performance
Performance issues to address
 🎨 UI / UX
Issues related to UI / UX interactions and design.
 💎 Bounty
💰 Rewarded
📊 Data issue
Issues where the app functionally works, but the data is missing or inaccurate.
 🙋 Bounty claim
$1.5K
Community
This is a great issue for community members to work on
 dependencies
Pull requests that update a dependency file
 Design
Hosted only
Hosted app only
 Required self hoster data migration
Self hosters must run data migrations to resolve this issue
 ruby
Pull requests that update Ruby code
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
gavin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gavin/maybe#1065
Delete Branch "1051-add-stimulus-tooltip-controller"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?