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

Migrating backdropscreensaver, filterdialog and fetchhelper to ES6 #1089

Merged
merged 12 commits into from Jun 7, 2020

Conversation

cromefire
Copy link
Contributor

@cromefire cromefire commented Apr 15, 2020

Changes
Migrating:

  • src/components/backdropScreensaver/plugin.js
  • src/components/filterdialog/filterdialog.js
  • src/components/fetchhelper.js

Issues
Relates to: #868

@heyhippari heyhippari added the es6 label Apr 25, 2020
@ferferga ferferga changed the title Migrating files to ES6 Migrating plugin.js to ES6 May 5, 2020
@ferferga
Copy link
Member

ferferga commented May 5, 2020

@cromefire I changed the title to a more descriptive one, so it's easier to sort among all the issues. Hope it's fine 😊.

@cromefire
Copy link
Contributor Author

Well there was planned to be 3 more files in here if I find some time again

@ferferga
Copy link
Member

ferferga commented May 5, 2020

@cromefire Feel free to change it once they are here. But, anyway, I would stick with adding the modules' names to the titles, all other PRs had it and it makes it easier to track. No worries if it's long (as long as GH doesn't break :P)

@cromefire cromefire changed the title Migrating plugin.js to ES6 Migrating plugin.js and filterdialog.js to ES6 May 5, 2020
@cromefire cromefire changed the title Migrating plugin.js and filterdialog.js to ES6 Migrating plugin.js, filterdialog.js and fetchhelper.js to ES6 May 5, 2020
@cromefire cromefire marked this pull request as ready for review May 5, 2020 22:06
@cromefire
Copy link
Contributor Author

Not really tested manually yet, if someone could either test it or tell me what to test

@cromefire
Copy link
Contributor Author

I can't find much more errors, review pls

@heyhippari heyhippari changed the title Migrating plugin.js, filterdialog.js and fetchhelper.js to ES6 Migrating backdropscreensaver, filterdialog and fetchhelper to ES6 May 6, 2020
@heyhippari
Copy link
Contributor

We have like 6 files called plugin.js, so I renamed it again to make it more clear :)

I can review and test it tonight.

@cromefire cromefire requested a review from dkanada May 8, 2020 09:49
src/components/fetchhelper.js Outdated Show resolved Hide resolved
src/components/fetchhelper.js Outdated Show resolved Hide resolved
src/components/backdropscreensaver/plugin.js Outdated Show resolved Hide resolved
src/components/backdropscreensaver/plugin.js Outdated Show resolved Hide resolved
src/components/filterdialog/filterdialog.js Outdated Show resolved Hide resolved
src/components/filterdialog/filterdialog.js Outdated Show resolved Hide resolved
src/components/filterdialog/filterdialog.js Outdated Show resolved Hide resolved
src/components/filterdialog/filterdialog.js Outdated Show resolved Hide resolved
src/components/fetchhelper.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/components/backdropScreensaver/plugin.js Outdated Show resolved Hide resolved
Copy link
Contributor

@heyhippari heyhippari left a comment

Choose a reason for hiding this comment

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

Looks good after Dmitry's comments are addressed.

@cromefire
Copy link
Contributor Author

Fixed
@MrTimscampi Please add the default dynamic import thing to the docs (in the epic)

@heyhippari
Copy link
Contributor

@cromefire Done.

This is good to go as soon as @dmitrylyzo approves the changes :)

Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

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

OK

The only thing left is that paramsToString adds an empty string param to the URL. There was a check for an empty string. In fact, fetchHelper isn't used with "params", so this may probably be omitted.

Test code
at the end of fetchHelper.js

console.debug(paramsToString({
    a: 2,
    b: 0,
    c: true,
    d: false,
    e: '123',
    f: '',
    g: null,
    h: undefined
}));

Output

a=2&b=0&c=true&d=false&e=123&f=

f= - empty string param

@cromefire
Copy link
Contributor Author

Will check for that too

@dkanada
Copy link
Member

dkanada commented Jun 5, 2020

@cromefire if you merge develop we can merge this.

@cromefire
Copy link
Contributor Author

So rebase on develop or what do you mean?

@cromefire
Copy link
Contributor Author

Or just on master again?

@dkanada
Copy link
Member

dkanada commented Jun 5, 2020

Oops I meant master.

@cromefire
Copy link
Contributor Author

Done

package.json Outdated Show resolved Hide resolved
Co-authored-by: Dmitry Lyzo <56478732+dmitrylyzo@users.noreply.github.com>
@cromefire
Copy link
Contributor Author

Hold this for a sec I can some errors testing it

@sonarcloud
Copy link

sonarcloud bot commented Jun 6, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 5 Code Smells

No Coverage information No Coverage information
5.5% 5.5% Duplication

@dmitrylyzo
Copy link
Contributor

Not sure about SonarCloud "code smells"

@cromefire
Copy link
Contributor Author

Okay forgot to pull, never mind...

Not sure about SonarCloud "code smells"

Yeah they smell... stuff. That function is explicitly written to do exactly why they "smell" (with the exception that I use a for (const loop, so it's definitely fine)

@cromefire
Copy link
Contributor Author

And I don't think I can set them as ignored

@anthonylavado anthonylavado merged commit 317838c into jellyfin:master Jun 7, 2020
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.

None yet

6 participants