-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add necessary scaffolding for enabling LV on dashboard #5930
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
Conversation
9d4c9c6 to
5a39247
Compare
5a39247 to
05f72e2
Compare
|
RobertJoonas
left a comment
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.
Impressive work! LGTM 👏
ukutaht
left a comment
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 to me!
Please add me as a reviewer in the future to these foundational PRs that establish liveview patterns for the dashboard.
| * Defines various widgets to use by various dashboard specific components. | ||
| */ | ||
|
|
||
| const WIDGETS = { |
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.
Personally I'm not a fan of introducing this new concept (widget). It took a bit of jumping around and reading the code to understand it's just a liveview hook. Why not call them what they are - hooks - and register them directly instead of via data-widget? This way anyone who's familiar with liveview (and the concept of hooks) understands immediately what's going on without having to learn and get used to an intermediary layer.
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.
By the way - I do like the shared structure for adding/removing listeners and cleaning them up. In Prima I have the same problem where all the components duplicate some amount of this structure.
I wonder if this can be solved via some kind of mixin or inheritance instead. I haven't really experimented with sharing behaviour between hooks yet.
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.
My worry was about having to add multitude of hooks, majority of them concerned with dashboard specific stuff. I'm not saying it's rational 😅 And you are right, it's not worth introducing cognitive load of a new concept I came up with "on the go". I'll see what can we do about structure sharing.
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.
My worry was about having to add multitude of hooks, majority of them concerned with dashboard specific stuff. I'm not saying it's rational
Haha yeah I don't think there's a problem with having many hooks. Surely hooks is just a map in liveview that looks them up in the same way as WIDGETS[this.widget].
It is a bit annoying to have to manually import and register them all when initializing a liveSocket. But I think there's also value in being explicit rather than magical. For namespacing and organization perhaps we can make something like this:
// live_dashboard.js
import HookA from "./hooks/hook_a.js"
import HookB from "./hooks/hook_b.js"
import HookC from "./hooks/hook_c.js"
export default {
HookA,
HookB,
HookC
}
// live_socket.js
import LiveDashboard from './live_dashboard.js
let Hooks = { Modal, Dropdown, LiveDashboard } // these are given to LiveSocket during initialization
// dashboard_component.ex
<div phx-hook="LiveDashboard.HookA">There's also a new ColocatedHook feature in Phoenix. I haven't tried it, not sure if/how it would work if you want to import some shared helpers or dependencies in these co-located hooks for example.
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.
WDYT about the approach taken in #5937 ?
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 good!
| ~H""" | ||
| <.link | ||
| data-type="dashboard-link" | ||
| href={@url} |
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.
We'll probably want to change this to patch over href eventually to get pushstate routing instead of http-based routing. Is there something preventing us from using patch from the get-go?
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.
Hmm, I got the impression patch is heavily tied to pushState - I'll double check that assumption though, maybe I have misinterpreted something. Either way, keeping things link dashboard links abstracted should let as replace href with patch where needed during the final migration phase.
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.
Either way, keeping things link dashboard links abstracted should let as replace href with patch where needed during the final migration phase.
True, feel free to ignore this for now. I don't have much experience with live navigation but my impression is that:
patchcallshandle_params, keeps liveview mounted and socket connectednavigatemounts another liveview while keeping socket connectedhrefdoes a full http cycle so socket is re-connected and any liveviews have to remount
As I understand, href should only be used when navigating to a dead view or external resource.
But as you say, this can be easily changed when we switch navigation over to liveview completely at the end.
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 have changed it to patch (#5937) as it currently does not really matter - href is rendered in the markup either way and the click is "hijacked" by the navigation hooks, so it will still work.
| :if={@tabs != []} | ||
| id={@id <> "-tabs"} | ||
| phx-update={@update_mode} | ||
| phx-hook="LiveDashboard" |
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.
The optimistic loading is currently implemented via custom JS in the hook. I wonder if this could be achieved via Phoenix.LiveView.JS{} commands instead and whether that would be any cleaner in the end (requires trying it out).
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'll play around with this a bit
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 have some ideas but I don't think it's important to focus on this right now. I'll implement these ideas in an upcoming Prima.Tabs component separately and plan to use it here as well.
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.
One idea might be prudent to talk about now.
As a general principle I've started preferring to style different states via html attribute rather than shuffling classes around. Quite common with tailwind. Here's what I mean:
Currently when a tab active state changes, the css classes are changed as well via:
def tab(assigns) do
assigns =
assign(assigns,
active_classes:
"text-indigo-600 dark:text-indigo-500 font-bold underline decoration-2 decoration-indigo-600 dark:decoration-indigo-500",
inactive_classes: "hover:text-indigo-700 dark:hover:text-indigo-400 cursor-pointer"
)
~H"""
<button
class="rounded-sm truncate text-left transition-colors duration-150"
data-tab={@value}
data-label={@label}
data-storage-key="pageTab"
data-active-classes={@active_classes}
data-inactive-classes={@inactive_classes}
phx-click="set-tab"
phx-value-tab={@value}
phx-target={@target}
>
<span class={if(@value == @active, do: @active_classes, else: @inactive_classes)}>
{@label}
</span>
</button>
"""
endvs my preferred approach where the class list is static and styles the component via data-attribute:
def tab(assigns) do
~H"""
<button
class="rounded-sm truncate text-left transition-colors duration-150"
data-active={@active} // <- this is used for styling
data-tab={@value}
data-label={@label}
data-storage-key="pageTab"
phx-click="set-tab"
phx-value-tab={@value}
phx-target={@target}
>
<-- active classes are prefixed with `data-active:` -->
<span class="hover:text-indigo-700 dark:hover:text-indigo-400 cursor-pointer
data-active:text-indigo-600 data-active:dark:text-indigo-500 data-active:font-bold
data-active:underline data-active:decoration-2 data-active:decoration-indigo-600
data-active:dark:decoration-indigo-500">
{@label}
</span>
</button>
"""
endIt doesn't work exactly the same because the base/inactive classes are not removed when state changes. So for example you might need to also override data-active:cursor-normal to get the original cursor styling for active state.
For me when working with styling I do prefer to have a static list like this to work with. It's especially helpful when debugging styling with devtools in the browser. Also using the data-attribute to announce different states is quite useful for testing and debugging.
@sanne-san WDYT?
| slot :inner_block, required: true | ||
|
|
||
| def tile(assigns) do | ||
| assigns = assign(assigns, :update_mode, if(assigns.connected?, do: "ignore", else: "replace")) |
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 understand why we ignore LV patches but what's the purpose of using phx-update="replace" when socket is not connected?
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.
replace is the default (there's no empty value). Though, now that I think of it, dead view most likely does not care about phx-update attribute at all, so it can be always set to ignore. I'll change that, thanks.
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.
Yeah that's exactly what I was thinking 👍
| def render(assigns) do | ||
| ~H""" | ||
| <div id="live-dashboard-container" phx-hook="LiveDashboard" data-widget="dashboard-root"> | ||
| <.portal_wrapper id="pages-breakdown-live-container" target="#pages-breakdown-live"> |
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.
Clever!
|
|
||
| ~H""" | ||
| <div data-tile id={@id}> | ||
| <div data-tile class="w-full flex justify-between h-full"> |
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.
Why the duplicated and nested data-tile attributes?
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.
That's a brainfart, I'll fix it.
| attr :id, :string, required: true | ||
| slot :inner_block, required: true | ||
|
|
||
| def tabs(assigns) do |
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.
Is this used?
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.
Ah, it's dead code from earlier iteration, I'll remove it, thanks.
I think I have added you as a reviewer - I haven't waited for your review though - sorry about that. Either way, things here are going to undergo ongoing improvements as we go anyway. |
Oh you did, I just didn't review in time :) No worries |
Changes
This PR introduces basics for supporting gradual deployment of LiveView site dashboard elements.
phoenix_live_viewdependency with added switch allowing disabling pushState calls (the branch is based off v1.1.18 tag)live_dashboardfeature flagreactor-dom-router; respective event dispatching and handlers are added on React end innavigation/use-app-navigate.tsxand indashboard/index.tsxLive.DashboardLV is now embedded in dashboard stats view, behind a feature flag<Pages />react component viaLiveViewPortalcomponent, also behind a flagTests