Skip to content

Conversation

Hachikoi-the-creator
Copy link
Contributor

@Hachikoi-the-creator Hachikoi-the-creator commented Nov 6, 2022

Related Issue/Addition to code

Fixes #16
Fixes #22
Fixes #23
Fixes #30

  • Totally new file layout(not sure how to phrase it)
  • Merged all the styles in 2 css files
  • Merged all the .js files in modules & implemented a new Class for code reusability
  • Fixed the onClick handler in /pages/topics.html
  • Moved all the data to their own js file & using an index.js to import them all and export theme easily
    • new naming convention whit the exports ex: footballQA = All the data needed for /football/game.html
  • Now every /pages/*/game.html has a little bit of JS on top, since it was the best approach I could think of, while also improving the perfomance
  • Litle improvement in markdown in pages/rules.html
  • Also changed hwdyk to kendall_quiz, hope it's not a problem

Type of change

live branch

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Proposed Changes

  • improve sintax in:
    • pages/rules.html
    • pages/highscores.html
    • pages/rate.html
  • Add a cute little favicon to all html files
  • In main quiz change the position of the current answers, since every answer is in index[1] or just suffle them all in the QuizClass.js

Additional Info

  • Now every game.html file wil have a little script on top, to avoid creating N files whit just 7 lines of code

Checklist:

  • My code follows the style guidelines of this project and have read CONTRIBUTING.md
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

Dev notes

Even tho it was a lot of work, I truly loved how consistent you where in the styling and classes naming, really makes the difference!

@Hachikoi-the-creator
Copy link
Contributor Author

This pretty much covers issues 16, 23 and 22
I got a little too exited lol

@KendallDoesCoding KendallDoesCoding self-requested a review November 6, 2022 14:44
@KendallDoesCoding
Copy link
Owner

Woah, that's a huge PR. 301 files, and it covers 3 issues. Thanks a lot! I will review and get back to you.

Copy link
Owner

@KendallDoesCoding KendallDoesCoding left a comment

Choose a reason for hiding this comment

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

It looks really good. But what happened to the main quiz.. I see that the main quiz just takes you to Browsers Quiz.
Main Quiz Preview: https://quiz.kendalldoescoding.tech/game/

Add a cute little favicon to all html files
I cannot see the favicon that you've said you added in Proposed Changes.

@KendallDoesCoding
Copy link
Owner

It looks really good. But what happened to the main quiz.. I see that the main quiz just takes you to Browsers Quiz. Main Quiz Preview: https://quiz.kendalldoescoding.tech/game/

Add a cute little favicon to all html files
I cannot see the favicon that you've said you added in Proposed Changes.

Awaiting your reply on this @Hachikoi-the-creator

@Hachikoi-the-creator
Copy link
Contributor Author

I tried it on your deployment and sent me to browsers quiz, so I assumed that's how it worked, will check and add it!

@Hachikoi-the-creator
Copy link
Contributor Author

oh if I do another PR just adds itself to this (context)? nice
I found the quiz you were talking about! @KendallDoesCoding
hope you like the improvements!

@KendallDoesCoding
Copy link
Owner

oh if I do another PR just adds itself to this (context)? nice I found the quiz you were talking about! @KendallDoesCoding hope you like the improvements!

coz you updated the stuff to the same branch!!

Copy link
Owner

@KendallDoesCoding KendallDoesCoding left a comment

Choose a reason for hiding this comment

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

Looks good! But, would it be possible to include the question number on the side of "Question text" like in the original website?

@Hachikoi-the-creator
Copy link
Contributor Author

Got into a bootcamp and finally got some free time, it was easier than expected lol

@Hachikoi-the-creator
Copy link
Contributor Author

what about adding mongoDB? and updating the highscores to show top5 of every category? that'd be sick!

@KendallDoesCoding
Copy link
Owner

what about adding mongoDB? and updating the highscores to show top5 of every category? that'd be sick!

can you make a issue (feature request) on this?

Copy link
Owner

@KendallDoesCoding KendallDoesCoding left a comment

Choose a reason for hiding this comment

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

Perfect, but please resolve conflicts to go to merge. @Hachikoi-the-creator

@Hachikoi-the-creator
Copy link
Contributor Author

I tried leaving package-lock.json exactly as it was and there's still conflicts, so no idea what to do to fix it.
and topics & style is for the changes I did to reuse CSS instead of copy&paste everything, or maybe the same weird thing whit package-lock.json I don't know

  • Given how big the project is, to make future changes you need to clone the repo in desktop and then add it to vscode, otherwise the live server won't work, probably was the same problem before as well

@KendallDoesCoding
Copy link
Owner

I can't even merge from command line :(

@KendallDoesCoding
Copy link
Owner

I managed to commit the PR, but I couldn't make it so you committed all the files :(!

@KendallDoesCoding KendallDoesCoding changed the title Rework, breaking changes [MERGED] Rework, breaking changes Dec 10, 2022
@KendallDoesCoding KendallDoesCoding merged commit 7637f9f into KendallDoesCoding:main Dec 10, 2022
@KendallDoesCoding
Copy link
Owner

Hey @Hachikoi-the-creator ,

Sorry that it took almost a month for me to merge this. I was really busy.

I don't know how to thank you. This is such a amazing Pull Request, that I've needed to do from a long time. Thanks for putting in so much hard work!!

@ghost
Copy link

ghost commented Dec 10, 2022

Sider has detected 3 errors on analyzing the commit a0188ed.

If the errors persist even after retrying, the following actions may resolve them:

If you still have problems, feel free to ask us via chat. 💬


You can turn off such notifications if unnecessary.

KendallDoesCoding added a commit that referenced this pull request Dec 10, 2022
bymistake #29 removed the github directory. This commit adds it back.
@Hachikoi-the-creator
Copy link
Contributor Author

nono thanks for making the merge, I was really overwhelmed working whit C and needed something refreshing, this was so fun to do!

@KendallDoesCoding
Copy link
Owner

@Hachikoi-the-creator I just included your name in README.md for the big changes you did to this project. I'm going to begin work/adding more quizzes, etc on this project soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Categories not opening up [BUG] Website not opening locally Major restructure of folders, files, etc. Improve responsiveness

2 participants