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

First pass at a Reflux.js solution #1

Merged
merged 11 commits into from Oct 7, 2015
Merged

First pass at a Reflux.js solution #1

merged 11 commits into from Oct 7, 2015

Conversation

pindia
Copy link
Contributor

@pindia pindia commented Oct 6, 2015

Here's my first attempt at a solution with Reflux.js. I ended up going with a design where the UI requests and releases "cursors" which tell the store/action level what it should keep loaded, and what it should throw away and/or abort.

The way React can request and release cursors in the component lifecycle methods ends up being very elegant IMO. The part where it tracks the requested cursors to do the right thing gets a little hairier, and the stores and actions end up quite coupled in the end. I'd love to hear other thoughts on solving this.

@winstonewert
Copy link
Contributor

It doesn't look to me like you cancel the requests when Obi-Wan is at one of the planets. Perhaps I'm missing it?

@staltz
Copy link
Owner

staltz commented Oct 7, 2015

Hi @pindia I get error

XMLHttpRequest cannot load file:///Users/staltz/flux-challenge/submissions/pindia/app.jsx. Cross origin requests are only supported for protocol schemes: http, data, chrome, chrome-extension, https, chrome-extension-resource.

in the browser when opening your index.html. Can you add build & run instructions?

@pindia
Copy link
Contributor Author

pindia commented Oct 7, 2015

@winstonewert Ah interesting, I missed that requirement. Will look into how that can fit in.

@staltz Will do, looks like I was relying on my IDE's automatic "serve current directory on localhost" feature and it doesn't work over file://

@pindia
Copy link
Contributor Author

pindia commented Oct 7, 2015

@staltz Added run instructions, let me know if you still have trouble

@staltz
Copy link
Owner

staltz commented Oct 7, 2015

Thanks @pindia, but there are still some requirements to pass. For instance

  • The list cannot be scrolled down if the last known sith has no apprentice. The list cannot be scrolled up if the first known sith has no master.

If I click 3 times quickly on scroll up, I get an empty list that simply cannot load any data anymore.

screen shot 2015-10-07 at 17 57 07

@pindia
Copy link
Contributor Author

pindia commented Oct 7, 2015

@staltz Ah interesting, that requirement is a little ambiguous (if the "last known Sith" is loading, what happens?). I chose to allow it because in the demo GIFs, it seemed that the scroll buttons did not gray out while the last known Sith was loading, only when the last known Sith completed loading (Of course, we don't know whether it's going to be the last one until it's done loading). If preventing scrolling during loading is the desired behavior, it's a very simple change (a boolean on line 124), but does this interpretation sound right to you?

@staltz
Copy link
Owner

staltz commented Oct 7, 2015

If preventing scrolling during loading is the desired behavior, it's a very simple change (a boolean on line 124), but does this interpretation sound right to you?

That's what @winstonewert did in his implementation, but in my implementation I allow scrolling even during loading. I can merge in your submission, but the more interesting question is: how much more complicated would it be to allow scrolling during loading without making it possible to "break" the app (meaning: possibility to take it to an bad state with no recovery).

@pindia
Copy link
Contributor Author

pindia commented Oct 7, 2015

how much more complicated would it be to allow scrolling during loading without making it possible to "break" the app

Interesting suggestion. I think I pulled it off by replacing the hardcoded Boolean with reading the load state of the Jedi +- 3 away from the loading one (so a minimum of 1 loaded Jedi is always on the page).

Also hacked in stopping loading (scrolling was already stopped) when the current planet matches, though not nearly as elegantly as @winstonewert with the React-like diffing approach. Don't have time now but in the future will look at how that approach might fit with mine.

@staltz
Copy link
Owner

staltz commented Oct 7, 2015

Yes, it works now

staltz added a commit that referenced this pull request Oct 7, 2015
First pass at a Reflux.js solution
@staltz staltz merged commit a524fe6 into staltz:master Oct 7, 2015
@winstonewert
Copy link
Contributor

Alternately, for a more challenging interpretation, you could allow all the sith to scroll off the page. Then you'd have to request all the sith that were scrolled off one-by-one to figure out which sith you are displaying.

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.

None yet

3 participants