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

Use less custom plumbing code in kee-frame template #392

Merged
merged 13 commits into from Oct 23, 2018

Conversation

ingesolvoll
Copy link
Contributor

Creating a PR to have a discussion about what this template should contain. I made some changes to illustrate what I'm thinking. See my comments in diff below.

(assoc db ::route route)))

(rf/reg-event-fx
:nav/route-name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't fully understand the purpose of this event, is it a workaround for a bug in kee-frame?

Copy link
Member

Choose a reason for hiding this comment

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

This just allows to dispatch routes by their names, e.g: {:on-click #(rf/dispatch [:nav/by-route-name :home])}

@ingesolvoll
Copy link
Contributor Author

Also, changes here are not tested. How do I test the template locally?

@yogthos
Copy link
Member

yogthos commented Oct 17, 2018

Overall looks good, for testing you can do lein install from the template folder, and that'll put a version of the template in your ~/.m2/repository/luminus/lein-template/ folder. Then you can just make a new project with lein new luminus test-app +kee-frame.

@ingesolvoll
Copy link
Contributor Author

As I said on slack, this is considered done now, with some possible bug fixing and tweaking remaining. Let me know if something seems off, and I'll fix it. I'll test some more after work today.

@ingesolvoll
Copy link
Contributor Author

Tested re-frame and kee-frame generating now, looks good. A suggestion:

How about separating pure views into a view.cljs? I think it would make the template structure clearer.

@ingesolvoll
Copy link
Contributor Author

The view split was done in the last commit. Let me know what you think, do we keep it?

@yogthos yogthos merged commit 3a40c87 into luminus-framework:master Oct 23, 2018
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.

None yet

2 participants