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 profileURL and title hash parameters #19

Merged
merged 2 commits into from
Apr 15, 2018

Conversation

alangpierce
Copy link
Contributor

On init, we check the hash fragment for these parameters and load the URL. We
always show a loading state in that case rather than the landing screen.

When determining the title, an explicitly-specified title takes precedence,
otherwise we use the filename.

I also added an error state, currently only used for my new code, but possibly
there could be a more robust or widespread error handling approach in the
future.

On init, we check the hash fragment for these parameters and load the URL. We
always show a loading state in that case rather than the landing screen.

When determining the title, an explicitly-specified title takes precedence,
otherwise we use the filename.

I also added an error state, currently only used for my new code, but possibly
there could be a more robust or widespread error handling approach in the
future.
Copy link
Owner

@jlfwong jlfwong left a comment

Choose a reason for hiding this comment

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

Sweeeet. Couple inline comments then this should be good to go

renderError() {
return (
<div className={css(style.error)}>
<div>😿 Something went wrong.</div>
Copy link
Owner

Choose a reason for hiding this comment

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

Nice :P

hash-params.ts Outdated
title?: string
}

export default function getHashParams(): HashParams {
Copy link
Owner

Choose a reason for hiding this comment

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

I haven't really been using default exports for anything anywhere, and IIRC, it makes mocking things kind of a pain. Would you might switching this to just a regular exported function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Not sure about the details of the mocking you're talking about, but at least with the normal commonjs transform, it is possible to mock default exports, I'm pretty sure.

hash-params.ts Outdated
const components = hashContents.substr(1).split('&')
const result: HashParams = {}
for (const component of components) {
let [key, value] = component.split('=')
Copy link
Owner

Choose a reason for hiding this comment

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

const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, thanks.

application.tsx Outdated
@@ -343,8 +352,23 @@ export class Application extends ReloadableComponent<{}, ApplicationState> {
}
}

componentDidMount() {
async componentDidMount() {
Copy link
Owner

Choose a reason for hiding this comment

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

It's a little weird having this function which is part of React's APIs return a promise. Can you extract the code that does the hash loading into an async method and just invoke it from componentDidMount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I've done this in my own code without anything going wrong, but agreed it's a little fishy.

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.

2 participants