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

Schedule an operation after redraw? #1837

Closed
andraaspar opened this issue May 2, 2017 · 12 comments
Closed

Schedule an operation after redraw? #1837

andraaspar opened this issue May 2, 2017 · 12 comments

Comments

@andraaspar
Copy link
Contributor

andraaspar commented May 2, 2017

It would seem natural for m.redraw() to return a promise that resolves after the redraw is completed – ie. the DOM is updated.

Example use case:

function focusNextRow(e) {
	let nextRowJq = getRowJq(e).next()
	if (!nextRowJq.length) {
		createNewRow()
		m.redraw()
		// I want to be sure to have the next row ready here
		nextRowJq = getRowJq(e).next()
	}
	return focusTextInputInside(nextRowJq)
}

The above function could be called from various event handlers, and manages focus as well as creates a new row when necessary.

@dwaltrip
Copy link

dwaltrip commented May 2, 2017

It sounds like using onupdate on the component the parent component could work for this.

https://mithril.js.org/lifecycle-methods.html#onupdate

@andraaspar
Copy link
Contributor Author

andraaspar commented May 2, 2017

@dwaltrip That would mean breaking up the flow into an additional function, some data flag to trigger the operation and additional data properties to keep the context of this function. A better option is a Promise with setTimeout. An even better option is for m.redraw() to return a Promise.

@dwaltrip
Copy link

dwaltrip commented May 2, 2017

What does createNewRow do? Is that manipulating the data model?

Normally, m.redraw is not manually called. Is there a specific reason you are calling it manually here? Redraws occur automatically whenever a DOM event occurs. The view should know how to render itself based off the current state -- event handlers generally update the state, and then the view will re-render based off those data changes.

@pygy
Copy link
Member

pygy commented May 2, 2017

As mentioned in the chat, you can use an empty fragment with hooks to get a hook that fires after all the hooks of a given tree:

{view(){ return [
  m(tree),
  m.fragment({onupdate(){}},[])
]}}

http://jsbin.com/xapomezedi/edit?js,console,output

@andraaspar
Copy link
Contributor Author

andraaspar commented May 3, 2017

@dwaltrip Yes, createNewRow manipulates the data model.

I am calling m.redraw() because I need an extra input field in the DOM, and then I need to focus it.

I figured something like @pygy suggested could work. But it'd be ugly. I simplified the example above, omitted (here irrelevant) contextual variables that should also be saved for the focus to be set correctly. If I don't get a Promise here:

  1. I need to create state variables to hold all these values, as well as a flag that makes it clear that there is a focus request.
  2. I need to pass the state to the focusNextRow function.
  3. I need to handle a potential focus request in oncreate and onupdate, for which I will need an extra function again.
  4. The resulting code would not express that there can be only one focused item in the document. That could lead to weird bugs.
  5. And the resulting code would not be easy to follow, and much less flexible.

I'd rather do something like:

async function focusNextRow(e) {
  let nextRowJq = getRowJq(e).next()
  if (!nextRowJq.length) {
    createNewRow()
    m.redraw()
    nextRowJq = getRowJq(e).next()
    if (!nextRowJq.length) {
      for (let i = 0; i < 10; i++) {
        await new Promise(resolve => setTimeout(resolve, 10))
        nextRowJq = getRowJq(e).next()
        if (nextRowJq.length) break
      }
    }
  }
  return focusTextInputInside(nextRowJq)
}

This is much more legible, and much simpler. It is ugly though, and this would be much better:

async function focusNextRow(e) {
  let nextRowJq = getRowJq(e).next()
  if (!nextRowJq.length) {
    createNewRow()
    await m.redraw()
    nextRowJq = getRowJq(e).next()
  }
  return focusTextInputInside(nextRowJq)
}

This is the reason for my request.

@pygy
Copy link
Member

pygy commented May 3, 2017

Actually, my comment was meant to be posted in #1836, I mixed things up, sorry for the noise...

As for your issue, you want to set the focus on a part of your app if it exists, and create a new field then set the focus on it otherwise, is it correct?

Do you have keys on your items?

@andraaspar
Copy link
Contributor Author

@pygy Correct. I do.

@pygy
Copy link
Member

pygy commented May 3, 2017

It makes things a bit trickier then, but still, there may be a way to do it all from the views, by marking the item that last had the focus in the model, and setting the focus on the next one on redraw (whether or not it exists at the point the previous one is marked).

If it had been a non-keyed list, you could have relied on the traversal order of the nodes, but keyed lists may be traversed out of order by the render engine, so you'd need, on redraw, to mark the next/new item based on whether the previous in the list had the focus, and use oncreate/onupdate to vnode.dom.focus().

Does that make sense?

@pygy
Copy link
Member

pygy commented May 3, 2017

@andraaspar Here's an example of what I meant:

http://jsbin.com/qobuminoqi/edit?js,output

@dwaltrip
Copy link

dwaltrip commented May 3, 2017

Is there a reason the target input cannot be managed by mithril? Something like:

var Component = {
  oninit: function() {
    this.dataRows = getDataRows();
  },
  createNewRow: function(params) {
    var newRow = /* set up new row */;
    this.dataRows.push(row);
  },
  view: function() {
    var isInputReceivingFocus = /* conditions for this flag */;
    var isTargetRow = (row) => /* determine if this is the target row */;
    return m('div', [
      this.dataRows.map(row => {
        return m('input', {
          type: 'text',
          oncreate: function(vnode) {
            if (isInputReceivingFocus && isTargetRow(row)) {
              vnode.dom.focus(); // set the focus on the <input>, using DOM api on the actual element
            }
          }
        });
      })
    ])
  }
}

I'm not sure I fully understand the context and what you are trying to do.

@dead-claudia
Copy link
Member

I'll note that this has been suggested before (way to run code after a redraw), including by me.

@barneycarroll
Copy link
Member

Closing due to inactivity.

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

No branches or pull requests

5 participants