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

fix: sort the exported list (fix new line issue) #9

Merged
merged 6 commits into from
Oct 30, 2020

Conversation

yuanLeeMidori
Copy link
Contributor

@yuanLeeMidori yuanLeeMidori commented Oct 30, 2020

Hi, since PR #6 has been closed, I create this new PR for solving issue #5 with no new line in the very beginning. Thank you again for reviewing and testing my code.

@daltonmenezes
Copy link
Owner

@yuanLeeMidori Thank you, can you resolve the conflicts between this PR and the updated branch master with your previous PR?

@daltonmenezes daltonmenezes linked an issue Oct 30, 2020 that may be closed by this pull request
@yuanLeeMidori
Copy link
Contributor Author

Just reloved the conflicts. They should be good to merge. Let me know if there's anything I can do, thank you!

@daltonmenezes
Copy link
Owner

@yuanLeeMidori Thanks, it's almost perfect, I'm just wondering about the letters with accent. Can you improve the .sort() function to put for example, the A and À together before the next letter (B)?

This is the the current state:

A Babá
A Bruxa
A Casa de Cera
A Maldição da Mansão Bl
Bird Box
Watchmen - O filme
Would You Rather
Wynonna Earp
Zumbilândia
[REC]
À Espera de um milagreQuestão de Tempo
À Procura da Felicidade
À Prova de Morte
À Sombra do Medo
À Sombra do MedoPsicoseKung FuryUm drink no inferno

And it would be great if we got the following result:

A Babá
A Bruxa
A Casa de Cera
A Maldição da Mansão Bl
À Espera de um milagreQuestão de Tempo
À Procura da Felicidade
À Prova de Morte
À Sombra do Medo
À Sombra do MedoPsicoseKung FuryUm drink no inferno
Bird Box
Watchmen - O filme
Would You Rather
Wynonna Earp
Zumbilândia
[REC]

Or

À Espera de um milagreQuestão de Tempo
À Procura da Felicidade
À Prova de Morte
À Sombra do Medo
À Sombra do MedoPsicoseKung FuryUm drink no inferno
A Babá
A Bruxa
A Casa de Cera
A Maldição da Mansão Bl
Bird Box
Watchmen - O filme
Would You Rather
Wynonna Earp
Zumbilândia
[REC]

What do you think about it?

@daltonmenezes
Copy link
Owner

@yuanLeeMidori I believe this

.sort(Intl.Collator().compare)

resolves my previous point. Can you try it and update the sorts in the code with it please?

@yuanLeeMidori
Copy link
Contributor Author

Sorry I was sleeping. I've updated all sort() with Intl.Collator().compare.

@daltonmenezes
Copy link
Owner

@yuanLeeMidori Thanks. For some reason, the last items in the list are copied on the same line, like this

Would You Rather
Wynonna Earp
Z Nation
ZumbilândiaQuestão de TempoPsicoseKung FuryUm drink no inferno

where the result should be

Would You Rather
Wynonna Earp
Z Nation
Zumbilândia
Questão de Tempo
Psicose
Kung Fury
Um drink no inferno

Can you check it please?

@daltonmenezes
Copy link
Owner

@yuanLeeMidori my previous point can be fixed by doing this:

.then(data => {
      if (data.ratingItems.length > 0) {
          data.ratingItems.map(data =>
            rate[thumb](data) 
              ? list += '\n' + data.title + '\n' 
              : ''
          )
          currentPageNumber++
          list = [...new Set(list.split('\n').filter(Boolean))].sort(Intl.Collator().compare).join('\n')
          return thumbsListCreator(thumb, list, currentPageNumber)
      }
      return list
    })

The "Set" is to remove duplicated names. Try it and update the code please, also it would be great to make the code more consistent if you remove the semicolons and change the double quotes to single quotes. In a near future I'll put eslint/prettier to make our life more easier about it :D

@yuanLeeMidori
Copy link
Contributor Author

yuanLeeMidori commented Oct 30, 2020

Hi, I've updated them with the new Set to prevent the duplicate issue and change the coding style as well, thanks for the reminder.

However, since I don't know the Portuguese language, it is a bit hard for me to get the Portuguese TVShow/movie names and test the functionalities. I've tried to change the language setting but couldn't get the list as you show to test. Moreover, even if there are multiple names being copied on the same line, I can't quite tell it because I can't read the language.

Anyways, please let me know if there is anything I can do, thank you.

@daltonmenezes
Copy link
Owner

daltonmenezes commented Oct 30, 2020

@yuanLeeMidori No problem, thanks. But you forgot the

 list += '\n' + data.title + '\n' 

If you don't put that new line at the beginning, the result still keep that issue

@yuanLeeMidori
Copy link
Contributor Author

list += '\n' + data.title + '\n' is for the thumbs-list-creator? If it is, there wasn't a new line before I sent my PRs. Would you like me to add them? Not sure if I get what you mean.

@daltonmenezes
Copy link
Owner

daltonmenezes commented Oct 30, 2020

@yuanLeeMidori You right, the point is: to your updates works properly and resolve that issue, you have to add this new line in the beginning, otherwise it will not work.

Just update thumbs-list-creator.js files from this

 list += data.title + '\n' 

to this

 list += '\n' + data.title + '\n' 

@yuanLeeMidori
Copy link
Contributor Author

The my-list-creator.js or thumbs-list-creator.js? There is no data.title in my-list-creator.js.

@daltonmenezes
Copy link
Owner

@yuanLeeMidori I just typed the wrong file name, my apologies. The correct files are the thumbs-list-creator.js files

@yuanLeeMidori
Copy link
Contributor Author

No worries. I've tested again and updated them as you suggested. Thank you. Please let me know if there's anything I can do.

@daltonmenezes daltonmenezes merged commit 89370df into daltonmenezes:master Oct 30, 2020
@daltonmenezes
Copy link
Owner

@yuanLeeMidori Now it's perfect. Works like a charm on Chrome,Firefox and Opera. Thank you so much for your contributions. As soon as possible I'll release theses updates to the stores.

@yuanLeeMidori
Copy link
Contributor Author

No problem. Thank you for giving me the chance to contribute and help me improve my code. Looking forward to seeing the new version of the extension in the stores!

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.

sort My List export output
2 participants