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

Level loading #194

Merged
merged 26 commits into from
Apr 11, 2020
Merged

Level loading #194

merged 26 commits into from
Apr 11, 2020

Conversation

JStrebeyko
Copy link
Collaborator

@JStrebeyko JStrebeyko commented Apr 3, 2020

The PR makes it possible to:

  1. level saving to Firestore using a button on GameControls;
  2. level loading from Firebase by URL;

It required turning userStore into a Vuex store module rather than having it entirely decoupled. An additional field appeared there, to cover for an asynchronously loaded level data. In the future, this logic should be probably moved into a dedicated module, but as it was only one function, I kept it where the rest of our db fetching level is, so the userStore module.

There are some steps that should be tackled next, like styling the login page, linking to /savedlevels page, ensuring that game does not throw on level being loaded, ConsistentErrorHandling, LoadingIndication.

@JStrebeyko JStrebeyko mentioned this pull request Apr 3, 2020
3 tasks
@stared stared self-requested a review April 3, 2020 22:42
@stared
Copy link
Collaborator

stared commented Apr 3, 2020

It works for my private saves.

image

However,

  • shared are not accessible (they load something different); I care for them
  • in general, I expect levels to have a single id, like UryMi5vJmmv3ldf6TjOJ NOT 12/UryMi5vJmmv3ldf6TjOJ

Maybe later we can evolve that into username/UryMi5vJmmv3ldf6TjOJ so it is more descriptive. It is JSFiddle style, see https://jsfiddle.net/stared/1Lbuoxte/.

@KlemKlem - what do you think about good UI of level sharing?

@JStrebeyko
Copy link
Collaborator Author

JStrebeyko commented Apr 6, 2020

Thanks for the feedback.

changes

  • the user-made levels have single id;
  • the shared work as expected;

extras:

  • level lists are updated on user action;
  • menu has a link to/myaccount for logged in users, to /login otherwise;
  • there's a stub of notification component;
  • link to /savedlevels is now my account page;

considerations

  • As of the levels already saved in db, I think the changes might make them inaccessible.
  • Is the progress section of /myaccount page really needed?

side-notes

As of CSS linting change that took place in the meantime, merging it made me implement linting work by hand on numerous files, which was very tiresome (the --fix option listed warnings, but did not fix them). The linting rules should be tweaked; we want the App component to be free of the "scoped" attribute.

@stared
Copy link
Collaborator

stared commented Apr 6, 2020

I tried to load it, but get errors:

image

@JStrebeyko
Copy link
Collaborator Author

JStrebeyko commented Apr 6, 2020

Unfortunately, these errors make it hard to pin point the source of the problem, the last one stems out of a typo already fixed.

What's even worse, downloading the repo, checking out onto PR branch (as described here), dealing with credentials and serving, results in a webserver with the app properly saving and loading levels on my end.

Let me know what does Vuex inspection bring around (an error, level Object consoled out?), or perhaps if you have any other ideas about possible source of the discrepancy.

edit: the image above shows the last publicly shared level record in the old format. Please try loading any other level.

@stared
Copy link
Collaborator

stared commented Apr 6, 2020

@JStrebeyko I tested it again, and it works.

A few remarks:

  • yes, current setting on myaccount were demo, and should be removed
  • on the other hand, options such as volume level and animation speed should be savable
  • now I see that there are two databases; levels and shared_levels; there should be one (otherwise we get in a long list of problems, and potential problems/inconsistencies, from different ids to not being able to update, etc); as I think, right now we should use a single link (if private, it won't be shown)

For now, we don't need t care about backward compatibility. Not many things saved, and the level schema will change at some point.

@KlemKlem
Copy link
Collaborator

KlemKlem commented Apr 7, 2020

@stared re UI of level sharing - do you mean a public page where all the levels created by users are available? I would definitely go for something like a thumbnail of the whole board, maybe some hashtags / categories to choose that would describe the levels? Otherwise, it'll be impossible to browse.

@stared
Copy link
Collaborator

stared commented Apr 7, 2020

Right now, when we start things, "public by default" makes the most sense. We just won't list some. (So just filtering 'public' when showing.)

In the long run - I think something similar to https://jsfiddle.net/, with username/short_id, and thumbnails (and name, tags, and ability to create whole campaigns). For now, let's start with a simple list.

Copy link
Collaborator

@stared stared left a comment

Choose a reason for hiding this comment

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

I accept as it is. There are more things to do (all mentioned) but the current state is an improvement, and does not break stuff, so let's merge that. (Prefer not to wait, as it would be harder to merge it.)

@stared stared merged commit 7e2f183 into Quantum-Game:master Apr 11, 2020
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.

3 participants