search

Soundofdarkness / RuneBook

📖 Arcane Rune Pages manager for League of Legends 📜
MIT License
100 stars 21 forks source link

fix for #88 #89

Closed ghost closed 3 years ago

ghost commented 3 years ago

That should fix #88. I undid the old changes and solved it as follows:

All returns from "getPages" are centrally chased through a wrapper. So we have a central possibility to make changes to the returns of the pages. Conceivable would be sorting by name or similar.

But currently only one new property is added and that is the prepared rune page that will be sent to LoL later. Since this wide correctly sorted runes has this then also to the property is passed which is used for the display etc..

This should fix the problem not only for u.gg but for all plugins (in case it occurs again).

Soundofdarkness commented 3 years ago

In fairness, I did already fix #88 in commit 0366b388a1e403516a25bec2835e4cb70b26f743 and said function should be able to be used in any case if this happens for any other provider, since it just sorts any valid rune page for displaying, so I'm not sure if its necessary to revert this again. Or is prepareRunePage already doing that too? cant check atm

ghost commented 3 years ago

Yes prepareRunePage already sorts the runes (as LoL itself expects this). There was already a sorting function under utils. I migrated this to prepareRunePage in commit 0366b38. As you can see the sorting was also used on other sites (blitz.gg, lolalytics and u.gg). Therefore, I have now built the global, that just no site will have a problem with it and we do not have two sorting functions. That's why I undid your changes.

Soundofdarkness commented 3 years ago

Okay, nice, that works then ! Apologies if my initial response sounded rude, I was just confused, since I didn't notice that we already had sorting before I implemented it (again) Will test your changes tomorrow and then merge them. Thanks !

Soundofdarkness commented 3 years ago

Yup, looks good ! Going to merge this, test it and then directly release a bugfix release ! (sadly didn't manage to find the time yesterday, sorry)