-
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
WIP tracklist navigation via now playing buffer #143
base: ft/tracklist
Are you sure you want to change the base?
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.
Notes
Adding some notes to guide our direction.
We should be very clear with prioritisation and focus on functionality wire ups before any cleanups
the code is ugly because we were rushing but the code is close to completion in terms of functionality. Integrating it is the main task.
@@ -255,18 +379,49 @@ defmodule VyasaWeb.Context.Read do | |||
}) | |||
end | |||
|
|||
### @bala here's how the injection is being done, we can hardcode inject the tracklist id and cursor and things will work as intended |
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 where the injection is currently being done, the integration for this requires us to parse from URL to make this dynamic instead of this stubbed, static version of things
tracklist_cursor={@tracklist_cursor} | ||
tracklist_id={@tracklist_id} |
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 visual debug dump is intended to help with ensuring correctness of the first mount state.
previously, the state issues @ mount that we experienced were happening because of the way that the injection of tracklist id was being trackloaded.
this is expected.
the integration task is what completes the proper wire-up of this.
function addEmphasis(selectorId, className) { | ||
const element = document.getElementById(selectorId); | ||
if (element) { | ||
if (!element.classList.contains(className)) { | ||
element.classList.add(className); | ||
} | ||
element.scrollIntoView({ behavior: 'smooth', block: 'center' }); | ||
element.focus({ preventScroll: true }); | ||
} | ||
} | ||
|
||
function removeEmphasis(selectorId, className) { | ||
console.log("WALDO", {selectorId, className}) | ||
const element = document.getElementById(selectorId); | ||
if (element && element.classList.contains(className)) { | ||
element.classList.remove(className); | ||
} | ||
} |
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.
although the emphasis manager exists, this already does the job.
The wire up and pending tasks should prioritise other state management and sorting those aspects out rather than focusing on how the add emphasis works.
curr_tracklist = | ||
Vyasa.Bhaj.get_tracklist(tracklist_id) | ||
|> Vyasa.Repo.preload(tracks: [event: [verse: [:source, :chapter]]]) | ||
|
||
tracks = curr_tracklist.tracks | ||
old_selector_id = "foo_track_" <> Enum.find(tracks, fn t -> t.order === curr_cursor end).id | ||
selector_id = "foo_track_" <> track_id | ||
class_name = "emphasized-verse" | ||
|
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 part is an unclean way of finding out the current tracklist.
however, fixing this can be scheduled to be done later (and the state wire ups / url parsing should be prioritised)
reason being that, just like how the read context currently has the other fields like the cursor in its state, we can just keep current tracklist within the socket state to avoid this JIT querying entirely.
selector_id = "foo_track_" <> track_id | ||
class_name = "emphasized-verse" |
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.
currently thhere's a lack of coherence in the navigation targets.
this is the current integration task that is top priority.
lack of coherence:
-
the selector is dependent on the current content_action within the read mode
-
the two functions (one for :show_tracks and another for :show_verses) need to be wired up together.
^ this is the pending integration work.
once this is done, the navigation and everything will work
the emphasis and such already works
<div id={ "foo_track_" <> track.id } class="font-dn text-2xl"> | ||
TODO track foo_track_{track.id} view: <br /> {to_title_case(track.event.verse.body)} |
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 was a stubbed example of deterministic ids that could be targetted by the event handler and emphasized
it was supposed to be an example to follow for the content_action = :show_verses
we can clean this up and it's good to go
slot :trigger, doc: "The element that triggers the sheet to open." | ||
slot :content, required: true, doc: "The content of the sheet." | ||
|
||
def sheet(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.
this is redundant, not being used and can be rm
i'm currently just using modal wrapper directly
end | ||
|
||
@impl true | ||
def handle_event( |
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.
there's a state management bug here (that's why the double clicking is necessary)
easiest signal for this is to look at the vissual debug dump and realise the value for :is_queue_visible doesn't toggle correctly
two actions to consdier:
- clicking the button @ action bar
- click-out from the playback queue
i haven't investigated the state management bug actually, so not sure as to the cause of it yet
this is an annoyance, but it can be tolerated, we can wontfix it for the meantime and this is low priority.
tracklist_cursor: track_order, | ||
track_id: track_id, | ||
tracklist_id: tracklist_id, | ||
verse_id: verse_id, | ||
chapter_no: chapter_no, | ||
source_id: source_id, | ||
source: source |
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 could be cleaned up by passingthe tracklist around, i had issues with it because of the json encoding stuff, but didn't know that we could jsut do a struct-serialise of sorts and pass it around
this is a refactoring candidate, but once again we can prioritse that cleanup for later
<.debug_dump | ||
label="Loaded tracklist" | ||
tracklist_id={@tracklist_id} | ||
is_queue_visible={@is_queue_visible} |
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 debugdump signal i was talking about -- the toggling for this bool flag isn't entirely right
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.
@ks0m1c here are the key steps to getthis wired up
id: "read", | ||
event: :set_cursor_in_tracklist, | ||
tracklist_cursor: tracklist_cursor, | ||
track_id: track_id, | ||
tracklist_id: tracklist_id | ||
# verse_id: verse_id, | ||
# chapter_no: chapter_no, | ||
# source_id: source_id, | ||
# source: source |
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.
step 2: flat thing can just be :
- cursor
- tracklist
defp apply_action(%Socket{} = socket, :show_tracks, _params) do | ||
# FIXME: this is a static stub, for now, these can be injected via url params / slug | ||
tracklist_id = "fc4bb25c-41c0-447a-90c7-894d4f52b183" |
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.
step 1: inject correctly, allow url params to override
cursor
tracklist id
|
||
IO.inspect(tracklist.title, label: "showing tracks in tracklist") | ||
|
||
tracklist_loader = fn -> |
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 where the loader gets init, 2 things that can affect it:
- url params
- clicking a single tracklist from the list of tracklists
tracklist_id: nil, | ||
tracklist_cursor: 0, | ||
voice: nil, | ||
video: nil, | ||
tracks: nil, |
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.
Step 3: handle the initial load @ media bridge
comments:
- can be cleaned up to make it less flat.
a. the playback stuff (cursor + tracklist)
b. ui state stuff (is action bar visible, is queue visible)
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.
bala is suggesting a non-sync model for this that will be simpler to do
an user-triggerred event-emitter model
def handle_event( | ||
"setCursor", | ||
%{ | ||
"track_order" => track_order, |
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.
naturally this follows the cleaned up struct for play state
|
||
# Do something with the track_order and track_id here, e.g., update the cursor | ||
|
||
send(socket.parent_pid, %{ |
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.
step 4:
not sure if this is the best way to send event from media bridge to the action-type specific component @ read context, VIA the mediator
this should likely do the mutate* thingy that relies on the mod registry
@@ -321,6 +409,42 @@ defmodule VyasaWeb.MediaLive.MediaBridge do | |||
}) | |||
end | |||
|
|||
def handle_info( | |||
%{event: :load_tracklist, tracklist_loader: loader, tracklist_cursor: tracklist_cursor}, |
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.
@ks0m1c commented:
this can be simplified and moved away from a sync model
@@ -66,6 +73,13 @@ | |||
px-4 sm:px-6 lg:px-8 py-3 flex items-center justify-center" | |||
> | |||
<div class="flex items-center space-x-4 sm:space-x-6 lg:space-x-8"> | |||
<.action_bar_toggler | |||
is_action_bar_visible={@is_action_bar_visible} |
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.
step 5: UI
this toggle not done yet, just have to wire this up
nuance:
when in a hidden state, the icon for "show" should be visible on the bottom left or something
No description provided.