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

Further improve UX, feature update, implement editor functions #14

Merged
merged 54 commits into from
Jun 11, 2020
Merged

Conversation

henryzt
Copy link
Owner

@henryzt henryzt commented Jun 10, 2020

  • User authentication logic update, default anonymous login
  • Implement result uploading and display
  • Game start logic update
  • Refactor visualizer, making it pluggable and modular
  • Implement sheet editor

@henryzt henryzt requested a review from jieyouxu June 10, 2020 09:30
Copy link
Collaborator

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Left some minor nitpicks for you. Note that if you encounter any of my suggestion that suggests you to use optional chaining (?.) or nullish-coalesing (??) in .vue files, please ignore them as we never setup babel to transpile Vue.js inline JavaScript...

Comment on lines +34 to +37
data: function(){
return {
}
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems degenerate?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes but I left there for future use lmao (better not tho), will be cleaned at some point. Or now...

Comment on lines +25 to +28
okText: undefined,
cancelText: undefined,
titleText: undefined,
bodyText: undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
okText: undefined,
cancelText: undefined,
titleText: undefined,
bodyText: undefined,
okText: null,
cancelText: null,
titleText: null,
bodyText: null,

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is actually intended, as vue would consider undefined to, you know, undefined. If I pass null it will indeed clear all the display text to null, which is not what I wanted. Is it ok to set vars be undefined?

src/components/ModalGlobal.vue Show resolved Hide resolved
src/components/SheetTable.vue Outdated Show resolved Hide resolved
src/components/SheetTable.vue Outdated Show resolved Hide resolved
Comment on lines 129 to 136
function clean(obj) {
for (let propName in obj) {
if (obj[propName] === null || obj[propName] === undefined) {
delete obj[propName];
}
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems... peculiar. Why exactly is this needed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Well this is used before firebase's update document command, which will overwrite the existing db document. I would like to remove any null attribute to prevent any desired value to be overwritten. However this might also make the user unable to make the field they intended to make blank though...

This also produces an ES lint Error for..in loops iterate over the entire prototype chain, which is virtually never what you want. Use Object.{keys,values,entries}, and iterate over the resulting array no-restricted-syntax

Do you have any idea to solve this better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the attribute already has a null value, then why shouldn't it be updated? Unless this is how firebase designs their API? Asking since I'm not familiar with firebase.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It's the other way around, if the attribute has data, should null overwrite it or not, is null intended by the user or not. I might need to find a better solution T T

src/mixins/gameInstanceMixin.js Outdated Show resolved Hide resolved
src/routes/Auth.vue Outdated Show resolved Hide resolved
src/visualizers/SpaceVisualizer.vue Outdated Show resolved Hide resolved
src/visualizers/SpaceVisualizer.vue Outdated Show resolved Hide resolved
@henryzt
Copy link
Owner Author

henryzt commented Jun 10, 2020

Left some minor nitpicks for you. Note that if you encounter any of my suggestion that suggests you to use optional chaining (?.) or nullish-coalesing (??) in .vue files, please ignore them as we never setup babel to transpile Vue.js inline JavaScript...

Ohhhhhhhhhhhhhhhh that's why! I thought it's vue doesn't support those...

@henryzt henryzt marked this pull request as ready for review June 11, 2020 15:28
@henryzt henryzt merged commit 0854040 into master Jun 11, 2020
@henryzt henryzt deleted the dev branch June 11, 2020 15:35
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