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

[ticket/12982] JS refactoring #2879

Merged
merged 6 commits into from Sep 12, 2014

Conversation

callumacrae
Copy link
Contributor

https://tracker.phpbb.com/browse/PHPBB3-12982

I'm trying to keep opinions out of this PR (I'm not going to be that guy).

I've converted a load of code to the correct coding standards, and basically fixed most JSHint and IDE errors in the JS.

It also contains a few potential bug fixes such as undeclared variables, and in one case, a typo'd variable name—please don't delay this until 3.2 (that would cause horrible merge conflicts, too).

@Nicofuma Nicofuma changed the title [WIP] JS refactoring [ticket/12982][WIP] JS refactoring Aug 13, 2014
@callumacrae
Copy link
Contributor Author

The bulk of this is now finished, the rest will be minor stuff.

@Louis7777
Copy link
Contributor

I thought the following might be related to this PR. See discussion: https://area51.phpbb.com/phpBB/viewtopic.php?f=84&t=45411&start=30#p266263

We use unescape() (when loading jQuery from the local assets) which is a deprecated function. We could replace it with decodeURI().

@callumacrae
Copy link
Contributor Author

That's not relevant to this PR.

On 21 Aug 2014, at 13:08, Louis7777 notifications@github.com wrote:

I thought the following might be related to this PR. See discussion: https://area51.phpbb.com/phpBB/viewtopic.php?f=84&t=45411&start=30#p266263

We are using unescape() which is a deprecated function. We could replace it with decodeURI().


Reply to this email directly or view it on GitHub.

@callumacrae
Copy link
Contributor Author

!unset WIP

@phpbb-user phpbb-user removed the WIP label Aug 21, 2014
@Nicofuma Nicofuma changed the title [ticket/12982][WIP] JS refactoring [ticket/12982] JS refactoring Aug 21, 2014
@PayBas
Copy link
Contributor

PayBas commented Aug 28, 2014

Did anyone actually test all this yet?

@PayBas
Copy link
Contributor

PayBas commented Aug 29, 2014

Been testing this for a while now. Haven't found any obvious problems.

dark.fadeOut(phpbb.alertTime, function() {
div.hide();
$dark.one('click', function(e) {
$confirmDiv.find('.alert_close').unbind('click');
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why you are still using unbind() here instead of off()?

@marc1706
Copy link
Member

PR looks good apart from what I mentioned above.

@callumacrae
Copy link
Contributor Author

FIXED

@bantu
Copy link
Collaborator

bantu commented Sep 11, 2014

@marc1706 Want to merge this then? If not, who else is capable of reviewing this properly? @nickvergessen ?

@marc1706
Copy link
Member

Will review this today

marc1706 added a commit to marc1706/phpbb that referenced this pull request Sep 12, 2014
@marc1706 marc1706 merged commit cb7e154 into phpbb:develop-ascraeus Sep 12, 2014
@nickvergessen
Copy link
Contributor

This broke the timezone select ...
When you select another time, the timezone list contains all timezones instead of only the ones with the actual time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants