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

Porting ES6 widgets/rhythmruler.js #2763

Merged
merged 3 commits into from
Jan 26, 2021

Conversation

joykirat18
Copy link
Member

With reference to issue #2629

@joykirat18
Copy link
Member Author

@meganindya please review this.

Copy link
Member

@meganindya meganindya left a comment

Choose a reason for hiding this comment

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

Uncaught TypeError: Cannot set property 'Rulers' of null
    at RhythmRuler2Block.flow (WidgetBlocks.js:885)
    at Logo.runFromBlockNow (logo.js:1603)
    at logo.js:1477

js/widgets/rhythmruler.js Outdated Show resolved Hide resolved
js/widgets/rhythmruler.js Outdated Show resolved Hide resolved
js/widgets/rhythmruler.js Outdated Show resolved Hide resolved
js/widgets/rhythmruler.js Outdated Show resolved Hide resolved
js/widgets/rhythmruler.js Outdated Show resolved Hide resolved
js/blocks/WidgetBlocks.js Outdated Show resolved Hide resolved
@joykirat18
Copy link
Member Author

I have made the necessary changes. Please have a look! @meganindya

Copy link
Member

@meganindya meganindya left a comment

Choose a reason for hiding this comment

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

On clicking clear button:

rhythmruler.js:932 Uncaught ReferenceError: r is not defined
    at RhythmRuler._clear (rhythmruler.js:932)
    at HTMLDivElement.RhythmRuler.widgetWindow.addButton.onclick (rhythmruler.js:2467)

The tap a rhythm button:

rhythmruler.js:177 Uncaught ReferenceError: Cannot access 'frame' before initialization
    at __move (rhythmruler.js:177)
    at RhythmRuler.__startTapping (rhythmruler.js:192)
    at rhythmruler.js:141
Uncaught ReferenceError: cell is not defined
    at RhythmRuler.__endTapping (rhythmruler.js:196)
    at rhythmruler.js:171

On clicking the save button:

Uncaught (in promise) ReferenceError: rList is not defined
    at RhythmRuler._mergeRulers (rhythmruler.js:2211)
    at HTMLDivElement.RhythmRuler.widgetWindow.addButton.onclick (rhythmruler.js:2389)

@joykirat18
Copy link
Member Author

I have tested the code. Hope there are no more errors. @meganindya Please have a look.

@meganindya
Copy link
Member

Pls don't merge master branch in your PR branch. In case it is necessary rebase your branch on top of the HEAD commit of the master. I've done it for you here.

Copy link
Member

@meganindya meganindya left a comment

Choose a reason for hiding this comment

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

Looks OK. Pls move the constructor to the top, format (prettify) the code, and clean up stray code, e.g. console.debug statements.

@joykirat18
Copy link
Member Author

Done the changes and formatted the code. @meganindya

@meganindya
Copy link
Member

You did not reset your local branch to the changes I made. There are conflicts and repeated commits.

@meganindya meganindya force-pushed the ES6_rhythmruler.js branch 2 times, most recently from 88437b4 to b1926f0 Compare January 26, 2021 13:53
joykirat18 and others added 3 commits January 26, 2021 19:25
function to classes

function to classes 2
handling this and that

minor changes let/const

fix bugs

fix bugs
Copy link
Member

@meganindya meganindya left a comment

Choose a reason for hiding this comment

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

Fixed all linting problems and formatted the code

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