bhauman / devcards

Devcards aims to provide a visual REPL experience for ClojureScript
1.53k stars 116 forks source link

Improve Om Next cards story #91

Closed anmonteiro closed 8 years ago

anmonteiro commented 8 years ago

this patch improves the recently added support for mounting Om Next components into devcards by adding:

anmonteiro commented 8 years ago

Added comments regarding the needed changes and additions :)

bhauman commented 8 years ago

Thanks man!

bhauman commented 8 years ago

I'm hoping to see how the current methods of extension are failing you or if we can actually use them.

anmonteiro commented 8 years ago

Sorry, I don't understand your last comment; what do you mean?

bhauman commented 8 years ago

OK it was hard to see what you added because of the single commit.
But basically you added an alternate main option and a react-key.

Here is what I'm thinking so far. I will definitely pull in render-base with the alternate main and that react-key parameter. But you might want to keep the Om Next specific stuff in Om Next or a micro library for a little bit.

The reason is that getting the reload behavior you want during development is going to probably take some tweaking. If you keep the Om Next helpers in a separate lib then you can tweak the heck out of them until you get the behavior you want. Then once it's solid then we should consider putting it into Devcards. But its highly likely that there will be some high churn in the near future.

anmonteiro commented 8 years ago

@bhauman OK, just removed the Om Next specific parts and I'll work on pushing an Om Next specific micro-repository

anmonteiro commented 8 years ago

However, will need a release whenever this gets merged, if possible, so that I can take advantage of e.g. render-base

bhauman commented 8 years ago

Alrighty, trying to get to it today. Looks good though.

On Sat, Jan 23, 2016 at 11:25 AM, António Nuno Monteiro < notifications@github.com> wrote:

However, will need a release whenever this gets merged, if possible, so that I can take advantage of e.g. `render-base´

— Reply to this email directly or view it on GitHub https://github.com/bhauman/devcards/pull/91#issuecomment-174197874.

bhauman commented 8 years ago

Been working on this this evening. Found a regression where components that with local state are getting blown away. Trying to track that down first.

bhauman commented 8 years ago

This wasn't introduced by your patch. It happened a few versions back.

anmonteiro commented 8 years ago

no rush :)

bhauman commented 8 years ago

I think I obviated the need for the react-key parameter.

See this commit: 7778d1622aa245e8e141e7b6c6095e4f1c94bc13

I think you were having to do that to get around a bug. Do you want to try against master and see if things work without the react-key parameter?

anmonteiro commented 8 years ago

Will do and let you know.

However, I'd still need render-base to be its own, public thing in order to make an Om Next specific card (because they mount in a different way, so the main override would still be a thing). I'll update the patch accordingly

anmonteiro commented 8 years ago

@bhauman alright, your changes in 7778d1622aa245e8e141e7b6c6095e4f1c94bc13 are indeed working for me without the react-key parameter. I've rebased my branch on the current master and kept the changes regarding render-base.

bhauman commented 8 years ago

@anmonteiro this is the next thing on my priority list. Sorry for all the back and forth. But I really want to get extension right. Can you send me a link to a repo or a gist that has your current card implementations? I suspect that you should/could be using iDevcard and I'm curious about a couple of other things. I may refactor render-base a bit ...

anmonteiro commented 8 years ago

@bhauman I am using IDevcard but I don't want to render using DevcardBase. I want to have my own component (that knows how to mount an Om Next component) but still keep the same rendering behavior as in my render-base. Here's what I'm currently doing (using the devcards branch I've submitted in this PR).

bhauman commented 8 years ago

Thanks. Ok I see a good way forward.

bhauman commented 8 years ago

I'm going to make a couple of changes.