Add a tick indication beside current theme

devkennyy / rungeon

🏃‍♂️Open source, puzzle based adventure game in your browser

https://rungeon.live/

GNU General Public License v3.0

17 stars 21 forks source link

Add a tick indication beside current theme #142

Closed devkennyy closed 2 years ago

devkennyy commented 2 years ago

This was lost in the stage-remake merge and is a change I think we should have. Simply change the text content of the theme title to include a checkmark emoji beside it. Don't forget to clear it when a new theme is set.

Shabbyconnor commented 2 years ago

Okay, I'm mostly writing this so I can think it through. I am planning on working on this. The code for the checkmark is still there but has an invalid selector. The items in the popup used to have an id referencing the theme name, but this has since been removed. The checkmark setter still works but it's looking for an id that doesn't exist.

We can either bring back the ids or filter by class. I'm going to try the second option in the meantime

devkennyy commented 2 years ago

Ah so the code is still there. Let me know if you need any help!

Shabbyconnor commented 2 years ago

Okay. I've got a fix, but there's a decision we need to make. Currently, because of the merged stage-remake, there are two systems to set the theme. One at https://github.com/devkennyy/rungeon/blob/deb64859dfa9431264b2e19d53e05b4d1d00153d/public/scripts/main.js#L63

And one at https://github.com/devkennyy/rungeon/blob/deb64859dfa9431264b2e19d53e05b4d1d00153d/public/scripts/themes.js#L37

We can't have both because it throws an error (about defining a function twice)

I've gone ahead and made a fix involving the newer WIP part but that means we can't use the older one. @devkennyy @GruelingPine185 what are your thoughts? I'm not a fan of deleting code before the new system is completed, but we do have a repo so we can always look back.

In addition. Can we bring the IDs back for the popup elements? (It makes it a lot easier because the dropdown doesn't share the same classes as the popup items)

devkennyy commented 2 years ago

How did you reference code from other files, that's cool! As for my thoughts, use the one in themes.js; that's what it's for. Theme-wise the master branch is fine (as far as I know) so we're all clear to remove it. setTheme & getTheme can be removed. I was in the process of removing the unnecessary code from main.js and separating it into modules but I couldn't get it to work.

Can we bring the IDs back for the popup elements?

Giving them ID's again? That will make it a lot easier to manipulate so go ahead.

Shabbyconnor commented 2 years ago

Okay! I'll start debugging and finalizing this fix. I'll probably have it out tomorrow.

How did you reference code from other files, that's cool!

If you click on a line number, three dots show up. Then click on the dots and select "Copy permalink". Paste the link and have fun!

devkennyy commented 2 years ago

Okay! I'll start debugging and finalizing this fix. I'll probably have it out tomorrow.

No pressure, good luck and thank you!