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

Add class to js/widgets/temperament.js and port ES6 to syntax. #2773

Closed
wants to merge 4 commits into from

Conversation

daksh4469
Copy link
Member

Please review @meganindya .

Copy link
Member

@ricknjacky ricknjacky left a comment

Choose a reason for hiding this comment

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

Not an official review but, this port generates Multiple errors:-
Uncaught TypeError: this._logo is undefined temperament.js:109:29
Uncaught TypeError: that._logo is undefined temperament.js:69:13

Moreover the widget doesn't even load up.
Screenshot (525)

@ricknjacky
Copy link
Member

ricknjacky commented Jan 26, 2021

Also, there already is an existing PR that ports this file to ES6 and handles bugs as well check #2772

@daksh4469
Copy link
Member Author

So, should I work on this file further to resolve these issues or start working on a different one?

@ricknjacky
Copy link
Member

ricknjacky commented Jan 26, 2021

So, should I work on this file further to resolve these issues or start working on a different one?

The issues that I mentioned above, occurred because of the port you did. I suggest you take up testing the platform, dig for some bugs and if they exist, solve them like in #2771 and #2772. You can also solve some of the open issues with priority major tag. Given the state some of the files are in, certain nuances must be handled with utmost care

Suggestions while porting files:-

  • Eliminate all occurrences of that, var( DON'T replace their occurrences globally, scoping is to be taken into consideration)
  • They should be replaced with this and let/const respectively.
  • functions should have ES6 syntax.
  • Refer recent commits by other contributors to get an idea of what is expected
  • There are valuable insights shared in some PRs like here and a conversation here study them carefully, then port, test and create a PR.

There are chances you'll miss something somewhere while creating PRs, no issues, we all do. Community members will help you out, Good luck.

@daksh4469
Copy link
Member Author

Thanks for the insight, @ricknjacky.....
I would look into this and would surely become better.
I also wanted to know about community platforms/forums which I could join to engage with the community members.

@ricknjacky
Copy link
Member

Thanks for the insight, @ricknjacky.....

You're welcome :)

I also wanted to know about community platforms/forums which I could join to engage with the community members.

Most of the discussion is done on GitHub i.e. either on the issues tab or discussions tab of the repository and sometimes in certain PRs too. Please feel free to use them for your perusal.

p.s:- You can close this PR.

@daksh4469 daksh4469 closed this Jan 26, 2021
@daksh4469 daksh4469 deleted the daksh4469-master branch January 27, 2021 05:22
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