search

elamperti / OpenWebScrobbler

🎧 An open source web scrobbler for Last.fm
https://openscrobbler.com/
GNU General Public License v2.0
323 stars 37 forks source link

Get older scrobbles for the profile lists #82

Closed erezsob closed 5 years ago

erezsob commented 5 years ago

Closes https://github.com/elamperti/OpenWebScrobbler/issues/63

elamperti commented 5 years ago

Hi, thanks for taking the time to work on this!

I see the pagination component is focused only on user profiles. What do you think about making it agnostic? For example, a component which receives current page, total pages and a function through onPageChange. This way it would be reusable in other views, even if they are not showing scrobbles for a particular user, and it will allow each one to implement their own logic as needed.

It would be good if the state is initialised inside the constructor() method, for consistency with the rest of the code and to keep it compatible with older browsers.

erezsob commented 5 years ago

Make total sense to make the pagination component more generic. It was my original intention, but somehow slipped my mind.

Do you prefer passing the specific props directly from directly from ScrobbleList or making a ScrobbleListPagination component that will isolate the specific logic and usage, i.e. connect to the store, get data from deep inside the profile object, pass props, etc.?

As for your second point, no problem with using a constructor for consistency, but isn't create-react-app's Babel config make it either way compatible for older browsers? Either way, should the class properties arrow functions be converted to regular class methods and be binded in the constructor also for consistency or is it okay to leave them as is?

elamperti commented 5 years ago

I think that it would be easier -at least for now- to keep the list and pagination not bundled. The ScrobbleList as it is already receives many properties and bundling it with the pagination won't change that -the new component will require at least a similar amount of props-.

Considering the current views relay on their state (instead of the store) to know which user to show and/or handle what happens in itself, I'd keep the logic in there, even if it means almost duplicating some of the code.

Regarding your Babel observation: yes, you are right there!

Arrow functions and methods: as you said, it would be better -for consistency- to keep them as regular methods and bind them from the constructor. I hope it's not too much trouble :)

Note: I recently upgraded the i18n libraries and the code for translation has changed a bit (instead of translate is now withTranslation, for example). After your final changes, I'll submit you a PR or suggest here the required changes, so don't worry about it :slightly_smiling_face:

If you consider faster to discuss the PR via chat, HMU on Discord. Thanks again!

elamperti commented 5 years ago

Hi @erezsob, were you able to look into this? Let me know if there's anything I can do to help! (or if you don't have time, that's ok as well!) Thank you :)

erezsob commented 5 years ago

@elamperti really sorry for my delay. I have an extremely busy month and I just couldn't find the time so far to properly close this issue although I really want to deliver it.

I will try to find the time in the next couple of evenings, otherwise starting from the coming weekend I will be again away from my computer for another 2 weeks (but after that I will have again much more free time). Just so you'll know and plan accordingly however you choose to.

elamperti commented 5 years ago

@erezsob thanks for letting me know! Please don't feel pressured to do this if you have more important things to do. We'll get there eventually :)

Thanks again for your contribution; have a good week ahead!

elamperti commented 5 years ago

@erezsob Sorry it took me so long to get back to this. I'll merge it now and make some modifications so it works with the updated translations library. Thanks for the time and the effort you put in this feature! :grin: