Skip to content
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

Add live view to cells #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GlennGeelen
Copy link

@GlennGeelen GlennGeelen commented May 2, 2019

Make it possible to have cells as live view

This implementation is an addition to the normal use of ex_cell, so normal cells can still be used the same way.

How to use live cell

In your project on any template

<%= live_cell(YourLiveCell, @conn) %>

<%= live_cell(YourLiveCell, @socket) %>

<%= live_cell(YourLiveCell, @conn, arg1: "value") %>

<%= live_cell(YourLiveCell, @conn) do %>
  some children
<% end %>

So it works the same as the cell-helper except you need to provide a conn or socket.

Rendering the live cell

https://github.com/DefactoSoftware/ex_cell/compare/master...GlennGeelen:glenn/add-live-cells?expand=1#diff-7eaa5ae18d45c80081813ff6a1d8edc9R61
I chose to use the live_render/3 from Phoenix.LiveView so the cell will go through the mount and render function of the YourLiveCell.

It is also possible to change this to cell.render/1 because the YourLiveCell should always have the render/1 function. That way the conn or socket is not necessary to give to every live_cell. In my opinion this is not the way to go because the in that case it will skip the mount step.

@GlennGeelen GlennGeelen force-pushed the glenn/add-live-cells branch from 60dc18e to 4f35dbe Compare May 2, 2019 10:45
@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #88 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #88   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      7    +2     
  Lines          45     54    +9     
=====================================
+ Hits           45     54    +9
Impacted Files Coverage Δ
test/support/conn_case.ex 100% <100%> (ø)
lib/ex_cell/live_view.ex 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b87d2e...4f35dbe. Read the comment docs.

@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #88 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #88   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      7    +2     
  Lines          45     54    +9     
=====================================
+ Hits           45     54    +9
Impacted Files Coverage Δ
test/support/conn_case.ex 100% <100%> (ø)
lib/ex_cell/live_view.ex 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50e5bc9...61e571c. Read the comment docs.

@GlennGeelen GlennGeelen force-pushed the glenn/add-live-cells branch from 4f35dbe to 0340288 Compare May 2, 2019 11:02
@GlennGeelen GlennGeelen force-pushed the glenn/add-live-cells branch from 0340288 to 61e571c Compare May 2, 2019 11:03
@@ -2,4 +2,5 @@ defmodule ExCell.MockViewAdapter do
@moduledoc false
def render(cell, template, args), do: [cell, template, args]
def render_to_string(cell, template, args), do: [cell, template, args]
def live_render(conn, cell, args), do: [conn, cell, args]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could also make another file mock_live_view_adapter with this function, if that is preferred?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need IMO

@sn3p
Copy link
Collaborator

sn3p commented May 3, 2019

Awesome 💯

This might introduce some problems/side effects when using cell-js.
Imagine the following implementation:

views/page/template.html.leex

<%= live_cell(AlertCell, variant: @variant) %>
  <%= @warning %>
<% end %>

cells/alert/template.html.eex

<%= container({variant: assigns[:variant]}) do %>
  <%= @children %>
<% end %>

cells/alert/index.js

class AlertCell extends Cell {
  initialize() {
    this.variant = this.params.variant;
    this.text = this.element.textContent;
  }
}

If I understand correctly, when @variant and @warning are updated within the Live View, the cell DOM remains the same. So the cell javascript is still binded to the DOM.
But in the example above this.variant and this.text are not updated because the cell is already initialized. This is mostly implementation that needs fixing, except for this.params which I would expect to be updated.

Possible solutions

  1. Reinitialize the cell(s) using Builder.reload(). We probably don't want this because the javascript is still binded and we want to have control over init and updating. Also this could get heavy with lots of live updates.

  2. Dispatch an event or trigger a callback on the cell (not sure if we can hook into live view javascript).

    class AlertCell extends Cell {
      initialize = () => setVariables();
      
      onLiveUpdate = () => setVariables();
    
      setVariables() {
        this.variant = this.params.variant;
        this.text = this.element.textContent;
      }
    }
  3. Whatever we do, I think it's important that this.params is always up-to-date and in sync with data-cell-params attribute (probably want to do this in cell-js).

    So instead of this:

    this.params = Cell.getParameters(element);

    Do something like:

    get params() {
      return Cell.getParameters(this.element);
    }

    (untested, no clue if this works)

@sn3p sn3p requested review from jessedijkstra and EasterPeanut May 3, 2019 10:46
@jessedijkstra
Copy link
Contributor

The first approach:
As discussed, seems to heavily break the intended live cycle of CellJS nodes.

The second approach:
Very terse but also very clear of what happens when you update a DOM node. Maybe instead of onLiveUpdate, call it onDOMUpdate. CellJS has no notion about LiveUpdate.

The third approach:
If the third implementation would work, it would be the best way to implement this, however: the Javascript might not run other functions dependent on the data, which creates weird situations.

This requires a major overhaul of CellJS to be reactive and observing the variables the same way for example EmberJS does with observables.

Things like MobX will be required if this is the wish but introduces another paradigm: functional reactive programming.

Alternative approach:
More of an approach that is based on the second approach, but requires MutationObservers.

CellJS observes mutations on the DOM nodes and calls the onParamsUpdate (or something) function if any of the params have changed.

The onParamsUpdate is trigged with 2 arguments, the previous arguments, and the new arguments, a bit like how React does updates. The previous arguments can then be used to do teardown and after the callback has been called, the new params are to the value that the onParamsUpdate returns.

To further elaborate to allow async functions, the function could instead return a promise instead of a value that returns the new value to be set on the this.params. Not sure about this approach though. Will give it more thought.

This was referenced Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants