Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Using hamus.js #250

Merged
merged 4 commits into from
May 4, 2019
Merged

Using hamus.js #250

merged 4 commits into from
May 4, 2019

Conversation

hrigner
Copy link

@hrigner hrigner commented May 3, 2019

Fixes #217

@hrigner hrigner requested a review from peol May 3, 2019 13:13
@netlify
Copy link

netlify bot commented May 3, 2019

Deploy preview for catwalk-qlikcore ready!

Built with commit 087d0d2

https://deploy-preview-250--catwalk-qlikcore.netlify.com

Copy link
Contributor

@peol peol left a comment

Choose a reason for hiding this comment

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

👌 Just a few ideas, but LGTM to me

const model = useModel(app, createDefinition(field));
const layout = useLayout(model);
const [model, modelError] = useModel(app, createDefinition(field));
if (modelError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Stylewise, I would've gone with a one-liner on these to keep the logic a bit more condensed so it's easier to read if (modelError) throw modelError;

Just an idea: What about having a error-throwing hook for all of these?

Like:

const [model] = useErrorThrow(useModel(app, createDefinition(field));`

@hrigner
Copy link
Author

hrigner commented May 4, 2019

You mean sth like my last commit @peol? :)

@peol
Copy link
Contributor

peol commented May 4, 2019

Yes, that looks great 👍 Perhaps the naming could be better though, feel free to rename from my suggestion, it sounds a little weird now that I see it. Essentially what it does is giving you the result (resolved value) from a promise, so maybe useResolvedValue or something instead? 🤔

@hrigner
Copy link
Author

hrigner commented May 4, 2019

Hm, true. The throw error-thingie is just a nice side effect..

@hrigner hrigner merged commit bbec847 into master May 4, 2019
@hrigner hrigner deleted the use-hamus-js branch May 4, 2019 20:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use hamus.js
2 participants