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

Reformat controller #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

voodoos
Copy link
Contributor

@voodoos voodoos commented Jan 18, 2020

Not for merging yet

I would like to start a series of opinionated code cleanup:
Now that we use babel es6 features can be used without restrictions.

Changes

Change I made for now, only in controller.mjs

  • Use new class def for the controller.
  • Remove useless getter/setter: if you don't need fine control, default js setters and getters are ok
  • Change numerous if sequences by switches. This is very opinionated, I can rewind if needed.
  • Check with jshint and reformat with javascript prettier. We might want to add these tools to the build chain.

Questions

Most of the key shortcuts do nothing, is that expected ? (zoom, rotate etc)
What is the difference between "t" and "T" ?

@panglesd
Copy link
Owner

Yes, lots of reformat are needed :)

  • I agree with the use of class from es6,
  • I don't mind the switch instead of if, though for me they are the same,
  • rotate used to work. It would be easy to make it work again, but I want a better way of dealing with it first, and it's clearly not a priority
  • zoom works once you enter the first slip, that is after the first next(). This is because I haven't yet implemented a start() method to engine.
  • I need to rename showToC to buildToC and then write showHideToC which is what will be called by pressing t. Pressing T rebuilds the table of content in the DOM and should now be deleted.

@panglesd panglesd force-pushed the master branch 7 times, most recently from 970ebfb to bc47642 Compare January 9, 2024 10:11
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.

2 participants